Skip to content
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

DisplayOptions: "Inc"/"Dec" operations in the function bar #1610

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

hishamhm
Copy link
Member

For numeric entries in Display Options, this gives users a hint of which keys operate on the value, and allows the value to be increasable/decreasable using the mouse as well.

@Explorer09
Copy link
Contributor

Related: #737 and #745.

May supersede #745, but I don't understand why another pull request here.

@BenBE BenBE added the enhancement Extension or improvement to existing feature label Feb 19, 2025
@BenBE
Copy link
Member

BenBE commented Feb 19, 2025

I don't quite like the extra cleanup code needed for this version. Cf. #772 for a different approach to this.

@BenBE BenBE added this to the 3.5.0 milestone Feb 19, 2025
@hishamhm
Copy link
Member Author

May supersede #745, but I don't understand why another pull request here.

Ah, I didn't see there was already a PR for this. Feel free to discard this one. (You still might want to take note of b2e86aa.)

It honestly took me a few seconds until I remembered that the "-" key decremented the value (at first I tried pressing space multiple times assuming the value would wraparound or something). Since the original point of the function bar at the bottom was to make the UI naturally discoverable, I thought that the operations applicable to the selected item should appear there, like it happens in the Meters screen.

If that's already being taken care of, all the better! Cheers!

@BenBE
Copy link
Member

BenBE commented Feb 19, 2025

May supersede #745, but I don't understand why another pull request here.

Ah, I didn't see there was already a PR for this.

Happens. I had to look it up too.

Feel free to discard this one. (You still might want to take note of b2e86aa.)

I already cherry-picked that commit onto main.

It honestly took me a few seconds until I remembered that the "-" key decremented the value (at first I tried pressing space multiple times assuming the value would wraparound or something). Since the original point of the function bar at the bottom was to make the UI naturally discoverable, I thought that the operations applicable to the selected item should appear there, like it happens in the Meters screen.

Yes. And for some other screens the keys already were noted so this is somewhat for consistency.

If that's already being taken care of, all the better! Cheers!

All mentioned PRs are somewhat incomplete for various details. In #745 there was some memory leak which I tried to fix with #772, but that PR hit a NULL deref on the way.

What bugs me with this PR is the global state introduced by the ctor logic; I'd somewhat prefer if the function bar had a somewhat more specific lifetime than "well, when the program closes".

@hishamhm
Copy link
Member Author

What bugs me with this PR is the global state introduced by the ctor logic; I'd somewhat prefer if the function bar had a somewhat more specific lifetime than "well, when the program closes".

I copied this straight from the MetersPanel.c logic, as you can imagine! :)

I don't remember why that one is static (because it is shared by multiple panels, perhaps?). I guess for DisplayOptions the FunctionBar could be just owned by the Panel itself. Yeah, I like that, it's way less code.

@hishamhm
Copy link
Member Author

cherry on top: as per @Explorer09's suggestion in #745 I flipped the button order. Also makes it consistent with "- Nice" and "+ Nice" in the main function bar!

@hishamhm hishamhm force-pushed the display-options-inc-dec branch from ffff5d2 to 4c6011c Compare February 19, 2025 23:09
For numeric entries in Display Options, this gives users a
hint of which keys operate on the value, and allows the
value to be decreasable/increasable using the mouse as well.
@BenBE BenBE force-pushed the display-options-inc-dec branch from 4c6011c to 3b7aa07 Compare February 20, 2025 07:37
@fasterit fasterit merged commit d28659d into main Feb 20, 2025
34 checks passed
@hishamhm hishamhm deleted the display-options-inc-dec branch February 20, 2025 12:59
@hishamhm
Copy link
Member Author

Thanks, folks! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants