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

feat: add more items in macOS menu bar #5266

Merged

Conversation

mavjs
Copy link
Contributor

@mavjs mavjs commented Mar 25, 2024

This PR tries to address some of the items listed in the brainstormed list mentioned in #1077

  • "About Chatterino" item which when clicked, opens the SettingsDialog with the "About" tab selected.
image
  • "Help" menu bar option that has multiple items linking to Wiki, Github, Discord, when clicked they open the URLs with the default browser.
image
  • "Minimise" item (under "Window" menu bar option) which when clicked, minimises the entire Window.
image

- "About Chatterino" item which when clicked, opens the SettingsDialog with the
  "About" tab open.
- "Help" menu bar option that has multiple items linking to Wiki, Github, Discord
- "Minimise" item which when clicked, minimises the entire Window.
src/widgets/Window.cpp Outdated Show resolved Hide resolved
src/widgets/Window.cpp Outdated Show resolved Hide resolved
@Wissididom
Copy link
Contributor

Wissididom commented Mar 26, 2024

Not sure, but this is what I find when googling for the question if a minimize option is standard for mac: https://developer.apple.com/design/human-interface-guidelines/the-menu-bar#Window-menu

As far as I understand it is recommended, but not necessary to do, but I have no experience with Mac at all.

- Moving `LINK_*` constants into Common.hpp allows for future reuses in other
  source than just AboutPage.cpp.
- After refactoring some of the `LINK_*` constants, include Common.http in
  AboutPage.cpp to make use of them.
@Felanbird
Copy link
Collaborator

should be minimize for both the fact that Apple documents it spelt that way, and we have no references in our code spelt with an s

Copy link
Contributor

@github-actions github-actions 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/common/Common.hpp Show resolved Hide resolved
src/common/Common.hpp Show resolved Hide resolved
src/common/Common.hpp Show resolved Hide resolved
src/widgets/Window.cpp Outdated Show resolved Hide resolved
Co-authored-by: pajlada <rasmus.karlsson+github@pajlada.com>
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Overall this looks great! Thank you! I'll test this briefly on my macOS Sonoma system then I'm happy to merge this in

@mavjs
Copy link
Contributor Author

mavjs commented Mar 27, 2024

Overall this looks great! Thank you! I'll test this briefly on my macOS Sonoma system then I'm happy to merge this in

Thank you!

One thing that's low priority (I think) is about the use of Meta in the MenuBar, while in fact it should be Control. However, for this PR, I didn't want to change such keyboard shortcuts until we get some consensus about whether that change is desirable. :)

Referencing: https://doc.qt.io/qt-6/qkeysequence.html#details

Note: On Apple platforms, references to "Ctrl", Qt::CTRL, Qt::Key_Control and Qt::ControlModifier correspond to the Command keys on the Macintosh keyboard, and references to "Meta", Qt::META, Qt::Key_Meta and Qt::MetaModifier correspond to the Control keys. In effect, developers can use the same shortcut descriptions across all platforms, and their applications will automatically work as expected on Apple platforms.

@pajlada
Copy link
Member

pajlada commented Mar 29, 2024

I've tested this and everything works as expected 👍
I don't have edit rights on the PR, could you make this PR up-to-date with master @mavjs?

@Felanbird Felanbird enabled auto-merge (squash) March 29, 2024 19:27
@mavjs
Copy link
Contributor Author

mavjs commented Mar 29, 2024

I've tested this and everything works as expected 👍 I don't have edit rights on the PR, could you make this PR up-to-date with master @mavjs?

@pajlada oops, I toggled that off when I sent a PR. (Have re-enabled it.) Been a while since I have contributed to some upstream open source stuff, so played around with that option. 😛

Seems @Felanbird (thanks!) has taken care of making it up-to-date with master. 🙏🏽

@Felanbird Felanbird merged commit b6d75fd into Chatterino:master Mar 29, 2024
17 checks passed
@mavjs mavjs deleted the feature/mavjs-macos-contextual-menu branch March 29, 2024 20:23
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