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

Add new frame menu to status bar button #4582

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

ckaiser
Copy link
Collaborator

@ckaiser ckaiser commented Jul 31, 2024

Adds a menu with new frame options when right-clicking the "+" button in the status bar:

feature.mp4

It also adds a new plus sign icon, icon_add, since for themes that are using regular fonts the plus sign was off-center and smaller than usual, I've also adjusted the status bar's border since that was also affecting button sizing and their centering.

Example of a misaligned button using the Wild Night theme:
image

In order to make the feature work at all I added the RightClick signal to BaseButton, this IMO improves consistency across the app since before there were some buttons that did nothing when right clicked, and others where right click meant a full click, so if you were testing for example if a button had extra functionality (like the animation play button) by right clicking it, this could result in accidental operations, so this makes buttons behave in the way that users usually expect them to. Ideally these buttons that have extra right-click functionality should have some sort of visual indicator, other than the current tooltip-only solution.

My main justification for the inclusion of this feature is that it increases discoverability of additional new frame options, since that might not be fully obvious from just the button, and it helps as a stepping stone into using the keyboard shortcuts.

I agree that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA") as stated in https://github.com/igarastudio/cla/blob/main/cla.md

I have signed the CLA following the steps given in https://github.com/igarastudio/cla#signing

@ckaiser ckaiser requested a review from dacap as a code owner July 31, 2024 21:03
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/app/ui/status_bar.cpp Show resolved Hide resolved
src/ui/button.h Show resolved Hide resolved
@dacap
Copy link
Member

dacap commented Aug 1, 2024

Hi @ckaiser! Thanks for your PR! I'll take a look to this next week, meanwhile, at a glance, it looks like the dark theme change broke the pal_resize icon:

break-pal-resize-icon

And the + sign has two columns:

image

I like the right-click idea for this "+" button and probably a future feedback for "right-clickable" buttons. I'm not sure how to make it in a way that is not visually annoying, probably the feedback could disappear once we right-click a button (?).

@martincapello
Copy link
Member

I have just took a quick look to this PR and it looks great! Just noted a couple of things that I will add as comments in the code.

@@ -34,13 +34,15 @@ namespace ui {
WidgetType behaviorType() const;
// Signals
obs::signal<void()> Click;
obs::signal<void()> RightClick;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would rename this to "ContextClick" instead. I took a look at some gui frameworks (unity and android) and liked this name because it is more generic, or less implementation oriented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this in the interview with @ckaiser, he saw the RightClick on ButtonSet so it's based on the existing naming.

Anyway we could use ContextClick in a future when a tablet-like UI is introduced. Because there are other elements that use "right click" like

CTRL_RIGHT_CLICK = 0x00008000, // The widget should transform Ctrl+click to right-click on OS X.

But I guess several renames will be needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the addition was mostly for internal and user consistency, if everything is already mouse-focused there's going to have to be either some kind of abstraction layer on top that maps whatever other input exists (touchscreen/pen/etc) to what is essentially going to be a mouse, or there's gonna have to be a pretty big refactor of input stuff, the former is probably the easier route to start with since there's some UI conventions that'd take some more planning like holding buttons to trigger context menus or swiping gestures, but it's still kinda nice to be able to do those with a mouse too if available.

@0975973358

This comment was marked as off-topic.

@dacap dacap self-assigned this Aug 8, 2024
@dacap
Copy link
Member

dacap commented Aug 8, 2024

Thanks @ckaiser! 👍 I've just tested and it looks great, I'll merge this squashed right now.

@dacap dacap merged commit e0ff519 into aseprite:main Aug 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants