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

UI: Add space for right arrow in menu #8082

Merged
merged 1 commit into from Mar 21, 2023

Conversation

exeldro
Copy link
Contributor

@exeldro exeldro commented Jan 18, 2023

Description

Add space for right arrow in menu

Motivation and Context

When making custom menu I noticed that the right arrow was on top of the text.
It looks to me that it is caused by setting the padding for QMenu::item to 6px in the theme.
Now I override only the right padding for QMenu::item to 12px
Before:
image
After:
image

The dark and system theme don't set the padding and don't have the issue.

How Has This Been Tested?

On windows 64 by having only 1 menu item that has a right arrow.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Jan 21, 2023
@GeorgesStavracas
Copy link
Member

Does this behave properly with right-to-left locales?

@exeldro
Copy link
Contributor Author

exeldro commented Jan 24, 2023

Does this behave properly with right-to-left locales?

No idea, what would be a good way to test that?
The arrow is called QMenu::right-arrow in qt so I expect it to always be on the right

@GeorgesStavracas
Copy link
Member

I don't really know, sorry. On Linux, usually launching the application with an RTL locale (e.g. LC_ALL=he-IL.utf8 obs) would be enough, but I'm not sure if Qt will respect that.

@RytoEX
Copy link
Member

RytoEX commented Feb 15, 2023

Does this behave properly with right-to-left locales?

I came here to ask the same question. Ideally, I'd like to know how this behaves in an RTL language/locale context.

@exeldro
Copy link
Contributor Author

exeldro commented Mar 21, 2023

Tested right-to-left by adding program.setLayoutDirection(Qt::RightToLeft); to obs-app.cpp
Al seems to work correct:
image

@WizardCM
Copy link
Member

WizardCM commented Mar 21, 2023

It's worth noting this also occurs on the system tray context menu on Windows. Reportedly this is because none of the QMenu items have an icon/checkbox.

image

I've tested this PR on Windows and verified it improves the behaviour. I will be testing on macOS and Linux momentarily.

Copy link
Member

@WizardCM WizardCM left a comment

Choose a reason for hiding this comment

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

I have verified this does not introduce any negative side effects on Linux or macOS.

However, I vote for changing it to 20px.

image

@exeldro
Copy link
Contributor Author

exeldro commented Mar 21, 2023

Made it 20px
image image

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

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

LGTM from the screenshots

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Seems better than the current situation.

Looks good to me.

@RytoEX RytoEX merged commit b11d61c into obsproject:master Mar 21, 2023
12 checks passed
@RytoEX RytoEX added this to the OBS Studio 29.1 milestone Mar 21, 2023
@exeldro exeldro deleted the qmenu_right_arrow branch March 21, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants