-
Notifications
You must be signed in to change notification settings - Fork 34
Pinnable "Page Search" with Right click #528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Pinnable "Page Search" with Right click #528
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Most look good to me.
I am currently not able to check the actual behavior. So either someone else has to check, or I can do tomorrow.
| protected DropDownPanel? commonDropDown; | ||
| private SimpleDropDown? simpleDropDown; | ||
| protected DropDownPanel? pagesDropDown; | ||
| private SimpleDropDown? pagesSimpleDropDown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me (by names) what all of these dropdown are (doing).
I know it wasn't very clear before, but since we are touching this, it might be nice to clarify this a little
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked too much DropDownPanel x SimpleDropDown
I just needed to use separate dropdown for pages search to be able to open for example the dropdown for recipe column and keep the pages search opened - until now just one dropdown could be opened at the same time while reusing the single one shared DropDownPanel and SimpleDropDown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever's going on with commonDropDown vs simpleDropDown is quite tortured. I'd love to see that refactored, but it seems like a lot of work.
I would like to see the two new fields renamed to better explain why we need two fields for one dropdown.
| Rebuild(); | ||
| } | ||
|
|
||
| public bool ClosePagesListDropDown(ImGui targetGui, Rect target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function documentation would be appreciated for (new) functions. In this case even more, as it is unclear what the returned bool is suppose to represent.
(Later I figured, it seems to be true when the dropdown was not closed?)
| this. | ||
| Rebuild(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this. | |
| Rebuild(); | |
| this.Rebuild(); |
No idea if you meant to explicitly wanted/needed to include this, if not please just call Rebuild() instead
|
|
||
| return evt; | ||
| } | ||
| public static bool WithTooltipConditional(this ButtonEvent evt, bool condition, ImGui gui, string tooltip, Rect? rect = null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Version: | ||
| Date: | ||
| Features: | ||
| - Add Right-click option to show the Page Search dropdown pinned to keep it open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You read my mind 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several files in 315170d also got committed with mixed line endings. Again, I can fix that if you'd like.
The other thing I noticed is that the page tooltip isn't drawn when the search dorpdown is pinned. That's not necessarily wrong, but I expected to see a comment explaining why that happened.

| public float PollutionCostModifier { get; set; } = 0; | ||
| public float spoilingRate { get; set; } = 1; | ||
| public float spoilingRate { get; set; } = 1; | ||
| public bool isPagesListPinned { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in ProjectPreferences instead. I haven't added a comment about this yet, but Preferences is for things that don't require the solver to run again, and Settings is for things that do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching that.
Preferences is for things that don't require the solver to run again, and Settings is for things that do.
This info should go to the javadoc. I'll make a ticket for that just to not forget.
| if (!ClosePagesListDropDown(gui, gui.lastRect)) { | ||
| showPagesListDropDown(pin); | ||
| this.project.settings.isPagesListPinned = pin; | ||
|
|
||
| } | ||
| else { | ||
| this.project.settings.isPagesListPinned = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up to veger's comment about ClosePagesListDropDown, I think this would be better as
| if (!ClosePagesListDropDown(gui, gui.lastRect)) { | |
| showPagesListDropDown(pin); | |
| this.project.settings.isPagesListPinned = pin; | |
| } | |
| else { | |
| this.project.settings.isPagesListPinned = false; | |
| } | |
| bool isDropdownOpen = /*something*/; | |
| if (isDropdownOpen) { | |
| ClosePagesListDropDown(gui); | |
| project.settings.isPagesListPinned = false; | |
| } | |
| else { | |
| showPagesListDropDown(pin); | |
| project.settings.isPagesListPinned = pin; | |
| } |
| protected DropDownPanel? commonDropDown; | ||
| private SimpleDropDown? simpleDropDown; | ||
| protected DropDownPanel? pagesDropDown; | ||
| private SimpleDropDown? pagesSimpleDropDown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever's going on with commonDropDown vs simpleDropDown is quite tortured. I'd love to see that refactored, but it seems like a lot of work.
I would like to see the two new fields renamed to better explain why we need two fields for one dropdown.
|
I'd like to discuss if this feature is redundant. Do you really search the list of pages that often that you need it pinned? Is the usual list on top of the page not enough? |
|
I totally forgot that this search is even there (I usually use regular search on the Summary page). But when I tried the search feature, I noticed how annoying it is that is closes each time when clicking a result going to the page it belong to. With this is it much faster clicking the results in order to find what you intended to find. |
To me, the vertical tab list is the primary benefit, and always showing the search controls is incidental. On the other hand, if the vertical list is the primary benefit, there should be a program preference that lets the user move MainScreenTabBar to any of the four sides of the window, rather than a way to open the search in a way that it doesn't auto-close. Now that I'm thinking about this, I'm surprised I haven't seen a request to have the tabs listed on the left instead of the top. |
This pull request introduces a new "pinned" mode for the pages list dropdown in the UI, allowing users to keep the list open for easier navigation. It refactors dropdown management to support multiple dropdown types, adds conditional tooltip behavior, and ensures the pages list stays up-to-date with project changes like renaming or adding pages. The changes also improve user interaction by enabling right-click to pin the dropdown and updating localization to inform users of this feature.
UI/UX Improvements:
ProjectSettings.isPagesListPinnedand reflected in the UI.Dropdown and Tooltip Logic:
WithTooltipConditionalto show tooltips only when dropdown is not pinned, improving clarity for users.DropDownPanelandSimpleDropDown.Project List Synchronization:
Localization Update:
Minor Fixes and Refactoring: