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

Fix dialog button mismatch #2908

Merged
merged 5 commits into from Jul 28, 2016
Merged

Fix dialog button mismatch #2908

merged 5 commits into from Jul 28, 2016

Conversation

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jul 10, 2016

Fixes #2518

Added/designed missing icons for the other 3 buttons. All buttons are now manually added. The ordering is done relative to the OS, and I don't think that should get fixed since qt probably has a good reason for it.

Sreenshots:
Qt4:
screenshot from 2016-07-11 00 05 11
Qt5:
screenshot from 2016-07-11 00 04 34

Testing on as many platforms would be appreciated, I'll see to make some windows builds (@tresf, when you get back from Vacation, could you test on a Mac?)

@Umcaruje Umcaruje force-pushed the Umcaruje:buttons branch from b1d9cf5 to a13b21c Jul 10, 2016
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 10, 2016

Cool!

Maybe the order of Recover and Discard should be switched, either in the menu or the buttons?

Testing on as many platforms would be appreciated

Release Linux Mint 17.3 Rosa 64-bit
MATE 1.12.0
Qt4, classic and default.

recoverscreenclassic

recoverscreendefault

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 10, 2016

Maybe the order of Recover and Discard should be switched,

You mean Discard and Ignore?

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 10, 2016

You mean Discard and Ignore?

Yes.

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 10, 2016

screenshot from 2016-07-11 01 31 00
Done.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 10, 2016

Yup, It's a keeper!

@teeberg
Copy link
Member

@teeberg teeberg commented Jul 11, 2016

I was trying to reproduce on a Mac to post back a screenshot, but can't seem to get the dialog to open. Could you post instructions on how to get to that dialog? I tried both running lmms twice and to kill -9 it, but the dialog never shows up.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 11, 2016

I tried both running lmms twice and to kill -9 it, but the dialog never shows up.

This should work. If you run from the build directory you may have to reselect auto save as we now have a separate .lmmsrc file ( per folder or per build directory... don't quite remember )

@teeberg
Copy link
Member

@teeberg teeberg commented Jul 11, 2016

Oooh, sorry about that, I thought this was around the "Create backup file when saving a project". I didn't see the auto-save one before. So, here we go:

Mac OS X 10.11.5, Qt 5.6.1

image

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 11, 2016

@teeberg does the button order make sense to you, as a Mac user? Qt changes the button order depending on the OS, to provide a consistent look and feel over applications, but I'm not sure how applicable this is in this case.

@teeberg
Copy link
Member

@teeberg teeberg commented Jul 11, 2016

To me, it would feel more intuitive to have the buttons be in the same order as the text. Although that still requires me to map the button texts to their meanings.

How do you feel about making the descriptions the buttons? Instead of having a table with button names and descriptions and those four buttons at the bottom. I.e. have 4 big buttons, one on top of each other, with the descriptions instead of the short names, and completely lose the "Recover"/"Discard"/'Ignore"/"Exit" names.

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 24, 2016

How do you feel about making the descriptions the buttons? Instead of having a table with button names and descriptions and those four buttons at the bottom. I.e. have 4 big buttons, one on top of each other, with the descriptions instead of the short names, and completely lose the "Recover"/"Discard"/'Ignore"/"Exit" names.

I feel like this would be inconsistent with the rest of the software.

I think I'll just make all the buttons the same role, so we can have custom ordering on all platforms

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 24, 2016

Ok, setting all the buttons to AcceptRole doesn't work, produces opposite order on qt4 and qt5

@liushuyu
Copy link
Member

@liushuyu liushuyu commented Jul 24, 2016

I think I'll just make all the buttons the same role, so we can have custom ordering on all platforms

I am totally agree with this. And It seems that the biggest difference is on the Mac. 👍

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 24, 2016

When I set all the buttons to the same role, I get this:
screenshot from 2016-07-24 16 04 37
screenshot from 2016-07-24 16 00 43

Top is Qt5, bottom qt4

@Spekular
Copy link
Contributor

@Spekular Spekular commented Jul 24, 2016

@Umcaruje Umcaruje force-pushed the Umcaruje:buttons branch from 7cf67ff to 7602ce2 Jul 25, 2016
@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 25, 2016

Ok here are the changes I've done:

  • I've hidden the Exit button by making it captionless and flat, and reinstated a close button to the dialog, so now when you press esc or the close button, the dialog exits.
  • I've changed the order of the buttons and text, so there is a more consistent flow for users when looking at the dialog. Here's an article on the topic: http://uxmovement.com/buttons/why-ok-buttons-in-dialog-boxes-work-best-on-the-right/
  • The buttons now all have the same role, and I added an if statement to check for the Qt version, and add them in the reverse order for qt4.

Screenshots:

Qt4:
screenshot from 2016-07-25 02 28 17

qt5:
screenshot from 2016-07-25 02 30 16

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 25, 2016

Good call on dropping the exit button...

qt4
screenshot at 2016-07-25 02 47 25

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 25, 2016

qt4

Weird, that's opposite then what I have when I compile with qt4 ...

Maybe this is a bug with qt4 and elementary os, since it works ok on qt5.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 25, 2016

Version 1.1.90-g18439ed (Linux/x86_64, Qt 4.8.6, GCC 4.8.4)
Release Linux Mint 17.3 Rosa 64-bit
MATE 1.12.0

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 25, 2016

Ok, so I did some testing. On unity, the order on qt4 is correct:
screenshot from 2016-07-25 23 56 54
The order is also correct on Gnome.

But when I tested on Xubuntu, the order appeared reversed, just like in @zonkmachine's screenshot.

Anyways, I think this can't go any better. Some testing on Mac and Windows would be nice.

@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 25, 2016

screenshot from 2016-07-26 00 43 02

This is the final look, made the exit button not visible, so it centers properly.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 25, 2016

Tested! Cool, like the last shot but buttons centred.

@Umcaruje Umcaruje force-pushed the Umcaruje:buttons branch from e44b6b6 to 5851c06 Jul 25, 2016
@Umcaruje
Copy link
Member Author

@Umcaruje Umcaruje commented Jul 27, 2016

Self merging in 12 hours if there are no objections.

@Umcaruje Umcaruje merged commit c20c520 into LMMS:master Jul 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.