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

Port the MainWindow ui to a .ui file #724

Merged
merged 16 commits into from Jan 27, 2023

Conversation

leia-uwu
Copy link
Contributor

@leia-uwu leia-uwu commented Jan 9, 2023

Some stuff still needs to be done in the c++ side because qt designer is dumb
(The instance toolbar icon and name buttons, and some shortcuts are still manually added in MainWindow.cpp)

Looks almost identical, with some minor tweaks:

  • The instance toolbar has been changed to a WideBar, allowing for customization of actions.
  • The instance toolbar buttons are now fullwidth.
  • The close window action has been moved to the end of the file menu and now has an icon.
  • The help menu has some layout changes.

This also fixes some stuff:

  • Menus not having tooltips.
  • The top toolbar not connecting to the title bar in kde.
  • The instance toolbar separators looking weird after you move the toolbar.

Closes #594, closes #69, closes #473

@leia-uwu leia-uwu force-pushed the dot-ui-mainwindow branch 3 times, most recently from b2d4803 to 71901fe Compare January 9, 2023 20:38
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

this looks pretty good overall!

there's only a couple minor things that I think could be improved, outside the comments i've made:

  1. While you did fix the alt shortcuts on the WideBar, they still show up in the Right-click menu, even though you can't even use those without closing the menu :p
    image

  2. I'm not sure about having icons in that menu. In the instance WideBars, this was not an issues because the actions don't have icons, but here they do, and it becomes ambiguous whether clicking that option will toggle its visibility, or activate the action.

  3. Maybe the old right-click menu actions, now only accessible through the top bar and the news bar, should be merged with this new menu somehow. It is quite weird to not be able to hide the Instance toolbar from interacting with that toolbar, but be able to by interacting with other toolbars. Perhaps they could be separated into two different sections, and the one to toggle the actions visibility have a name that would clear up the ambiguity I talked about in item 2, so the icons could be left there :)

launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
@flowln flowln added bug Something isn't working enhancement New feature or request labels Jan 9, 2023
@flowln flowln added this to the 7.0 milestone Jan 9, 2023
@leia-uwu
Copy link
Contributor Author

leia-uwu commented Jan 9, 2023

  1. even though you can't even use those without closing the menu :p

uh, for some reason the menu only closes when you press alt while using the fusion theme in my testing, this also happens in the instance view context menu and in the menubar :/

@Scrumplex Scrumplex self-requested a review January 10, 2023 05:41
@leia-uwu
Copy link
Contributor Author

:catstare:
image

@DioEgizio
Copy link
Member

with this the status bar doesn't seem to follow my prism theme

this pr this pr

w/o this pr without this pr

@txtsd txtsd self-requested a review January 10, 2023 17:36
@TayouVR
Copy link
Member

TayouVR commented Jan 10, 2023

hmmm... its a different shade of grey and doesn't have the margins/paddings from the theme.
After scrolling through the code I can't spot any obvious reasons for that, maybe the various size constraints on the QStatusBar in the .ui are different from the defaults it had in code? 🤔
image

@leia-uwu
Copy link
Contributor Author

with this the status bar doesn't seem to follow my prism theme

Fixed

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

I couldn't find any differences, besides the news toolbar as @flowln already pointed out.
LGTM and thanks!

Copy link
Member

@TayouVR TayouVR left a comment

Choose a reason for hiding this comment

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

couldn't find any issues in my testing, looking good!

leia-uwu and others added 11 commits January 20, 2023 19:44
Signed-off-by: leo78913 <leo3758@riseup.net>
some stuff still needs to be done in the c++ side because qt designer is dumb >:(

the instance toolbar icon and instance name buttons are still added manually inside MainWindow.cpp

looks almost identical, with some minor tweaks:
- the instance toolbar is now a WideBar, so you can customize what actions you want :D
- the instance toolbar buttons are now fullwidth
- the close window button is now at the end of the file menu
- the help menu has some layout changes

this also fixes some stuff:
- menus not having tooltips
- the top toolbar not connecting to the title bar in kde
- the instance toolbar separators looking weird after you move the toolbar

Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
WideBar::insertSeparator was adding the separator to the end of the toolbar

Signed-off-by: leo78913 <leo3758@riseup.net>
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: leo78913 <leo3758@riseup.net>
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
this reverts it to how it was before the MainWindow .ui port

Signed-off-by: leo78913 <leo3758@riseup.net>
i forgor 💀

Signed-off-by: leo78913 <leo3758@riseup.net>
this makes the accounts button and menubar item share the same QMenu
and also refactors some code

Signed-off-by: leo78913 <leo3758@riseup.net>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

some nitpicks, but looks good overall!

launcher/ui/widgets/WideBar.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/WideBar.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.ui Outdated Show resolved Hide resolved
launcher/ui/MainWindow.ui Outdated Show resolved Hide resolved
@Edgars-Cirulis
Copy link
Contributor

Kind of annoying, after toggling, the check automatically closes the menu.

leia-uwu and others added 4 commits January 27, 2023 12:35
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: leo78913 <leo3758@riseup.net>
Co-authored-by: flow <flowlnlnln@gmail.com>
Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
Signed-off-by: leo78913 <leo3758@riseup.net>
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@flowln flowln merged commit c78db54 into PrismLauncher:develop Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the sidebar buttons Fix tooltip Widen instance management buttons
7 participants