[Manager] Gray out projects in project wizard that are already attached #5862
Conversation
The code in this commit is repeated, additionally it will be used in the same PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5862 +/- ##
============================================
+ Coverage 10.53% 10.70% +0.16%
Complexity 1068 1068
============================================
Files 279 280 +1
Lines 35867 36710 +843
Branches 8409 8515 +106
============================================
+ Hits 3779 3930 +151
- Misses 31694 32391 +697
+ Partials 394 389 -5 |
|
There should be a hint explaining the meaning of the different color, like: As an option or in addition something like this could be at the top of the "Project details" page: There may also be a new entry in the "Categories" drop-down list like: |
|
@Vulpine05, please fix linux build |
|
Hmm...I may have to go back to the drawing board on this one. Apparently Owner Drawn is a feature in wxWidgets for Windows build, but not for mac nor Linux. I suppose I could add precompiler code to use this feature only for Windows, but that kindof defeats the purpose of the PR if it can only work on one platform. |
When you click on the Next button a message does pop up stating that the project is added. I am assuming the user has some knowledge of what project(s) they are attached to, so I am hoping that is obvious enough. Your idea isn't bad, but in my opinion I would rather keep the window cleaner.
Neat idea, but may be not as easy to add to the code. Again, just my opinion, so if there is strong support for this, I'd be willing to work on it. |
Using wxLB_OWNERDRAW is not compatible with Linux. Changing the class from wxListBox to wxListCtrl provides a differnt way of changing the text color for each row and still provide functionality across platforms.
|
Okay, this should be ready for review. I only tested this on Windows, so if someone can test on Mac and Linux it would be appreciated. @CharlieFenton, I have one concern with running this on Mac. Per the wxWidgets manual here, it says that report mode for a wxListCtrl is generic for all modes. I'm not sure exactly what that means. Could you see how it looks? Here is how it looks to me in Windows: |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- clientgui/ProjectInfoPage.cpp: Language not supported
- clientgui/ProjectInfoPage.h: Language not supported
The implementation on Windows uses a built-in API in the Windows OS. There is no such API on the Mac (and perhaps other platforms), so wxWidgets provides a generic implementation in code to be used on those platforms, simulating what the Widow API does. II tested it, and it works on the Mac, more or less. while the projects to which you are already attached are shown in a lighter gray color, they are not disabled. You can still select them and click on the Next button, which then tells you you are already attached to the project. Here aer 3 screen shots. 
|
|
I assume the title of this PR "Gray out projects in project wizard that are already attached" means that the intent is to disable those projects in the list so that attempted clicks on the are ignored, which is what a gray item in a list normally means. This does not accomplish that goal, but only changes the appearance ofthose projects in the list. However, it probably works well enough since clicking on the item brings up the "already attached" alert. |
Thanks for testing this and sharing screen shots, @CharlieFenton. I'm glad to see that it functions the same as in Windows. I understand how you and others assume that with the grayed out text then the next button should be disabled. However, the way the project wizard currently operates is that it checks the project URL field, which is editable. Although you can select a project and have the information filled out in the project details, the user can still modify the URL so they can add a custom project not on the list, such as SETI's former beta test project. Does this work well enough? I think so. Is there a better way? Probably, but there are enough guardrails to prevent the user from doing something that causes an error. |
I think so too. |
Fixes #1552
Description of the Change
This PR adds a feature to the Add Project wizard that will change the text of a project to gray if the project is already added. This is accomplished by storing a string array of all projects that are available in the wizard, another string array of all projects currently attached. Each array is then canonicalized, then trimmed of "http", etc. Arrays are then compared and if any match, the text is set to gray. The ability to make the text gray was enabled by adding the property "wxLB_OWNERDRAW" to the wxListBox.
For a "simple" change, this is actually a bit more complicated. A few design notes:
Adding the Ownerdraw property did more than provide the ability to change the text color. The background color of the wxListBox, wherenever there is text, also changed to grey. I discovered this was a bug and not a feature in wxWidgets, and have reported it here. It is being fixed with this pull request. I haven't tested the fix yet (I'm not sure I have the skill set to change BOINC's wxWidgets dependency). Therefore, if you were to built with this PR, it looks like this:
I had hard coded a white background before I noticed it was a bug with wxWidgets, and with that it looks like this:
Of course, that white background would be redundant once the wxWidgets PR is merged, so that hard coding is not in this PR.
I have not tested this in Linux nor MacOS. I also have not reviewed the code for anything dark mode settings. I will need someone to check on those platforms to see if I broke anything. FWIW, I run Windows in dark mode.
I am not an expert in accessibility, but a 4.6:1 contrast ratio looked good to me. I tried to get a 7:1 ratio, but the gray looked too close to black to me and was hard to notice the distinction.
From what I have worked on, I think this PR is ready, but I kindly request others to test this on other platforms, especially with dark mode, before merging.
Alternate Designs
Hiding the projects that are attached didn't seem like a good idea to me. If someone wanted to find out the information of the project in this wizard, they would not be able to unless they removed the project. By graying out the projects that are attached, the users is able to see which projects that are still active and they are attached to.
Release Notes
[Manager] Projects listed in Add a Project wizard that are already added are changed to grey.