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

DolphinQt2: Startup Warning #4006

Merged
merged 2 commits into from Jul 13, 2016
Merged

Conversation

EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 13, 2016

In PR #3981 Phire suggested adding a warning prompt to DolphinQt so that users don't accidentally try to use it instead of DolphinWX if both binaries are included in the packages produced by the buildbot. [The other option is to produce a separate build package for DolphinQt so the binaries don't end up mixed together.] This needs someone to supply the text for whatever it should say.

experimental


This change is Reviewable

@phire
Copy link
Member

phire commented Jul 13, 2016

Looks good.

Null VideoBackend is not being linked correctly.
@MayImilae
Copy link
Contributor

Ooooh ok, um, I need to give some pointers here!

The Dolphin is not vertically centered in the text block (all three paragraphs) it is in. It is also very tight, and needs much more padding around the logo!

Black and yellow stripes just looks... severe. And it seems rather excessive I think. Isn't it attention grabbing enough by presenting an unknown pop up in front of them?

There's like a sea of unused space on the bottom left! The buttons really should be placed in a row horizontally along the bottom, and not stacked vertically. It would be a much more efficient use of space in this design.

Windows XP arrows. Please no. PLEASE.

@EmptyChaos
Copy link
Contributor Author

@MaJoRoesch The icons are decided by the Qt theme. I use Windows Classic on Windows 7 so it uses Windows XP style icons. Here's how it looks using Aero:
experimentalaero

The use of hazard tape is intended to be intimidating so the user will opt not to proceed. I can remove it if you prefer it to be less extreme.

I can lay out the buttons horizontally instead but I think it reads better vertically since it leads the user to just click the top one. It's less obvious when laid out the other way. Windows UX prefers vertical layout for this type of button as well, I could remove the logo from the design to compensate for the wasted space or center the buttons under both the logo and paragraphs instead of just the text.

How much spacing do you want around the logo?

@lioncash
Copy link
Member

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions.


Source/Core/DolphinQt2/InDevelopmentWarning.h, line 7 [r3] (raw file):

#pragma once

#include <QBrush>

QBrush can be a forward declaration.


Source/Core/DolphinQt2/InDevelopmentWarning.h, line 10 [r3] (raw file):

#include <QDialog>

class InDevelopmentWarning : public QDialog
class InDevelopmentWarning final : public QDialog

Comments from Reviewable

@lioncash
Copy link
Member

Source/Core/DolphinQt2/InDevelopmentWarning.h, line 18 [r3] (raw file):

  ~InDevelopmentWarning();

  static QBrush MakeConstructionBrush();

These two static functions should also likely be private if possible.


Comments from Reviewable

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

Yellow banner LGTM.

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

I don't think we need the Dolphin logo here TBH. Would make the window smaller without.

@lioncash
Copy link
Member

Source/Core/DolphinQt2/InDevelopmentWarning.h, line 18 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

These two static functions should also likely be private if possible.

That or simply get rid of them in the class declaration and make them static free functions in the cpp file, since they're technically implementation details that aren't necessary in the visible interface at all.

Comments from Reviewable

@EmptyChaos
Copy link
Contributor Author

Review status: 0 of 10 files reviewed at latest revision, 3 unresolved discussions.


Source/Core/DolphinQt2/InDevelopmentWarning.h, line 7 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

QBrush can be a forward declaration.

Removed.

Source/Core/DolphinQt2/InDevelopmentWarning.h, line 10 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

class InDevelopmentWarning final : public QDialog
Done.

Source/Core/DolphinQt2/InDevelopmentWarning.h, line 18 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

That or simply get rid of them in the class declaration and make them static free functions in the cpp file, since they're technically implementation details that aren't necessary in the visible interface at all.

Moved to free statics.

Comments from Reviewable

@EmptyChaos
Copy link
Contributor Author

Without the logo:
experimentalnologo

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

Something weird about the buttons due to the button size being far wider than the text it causes the text to be more left than it should.

@EmptyChaos
Copy link
Contributor Author

I changed the proportions on the stretch factor:
experimentalaero2

The buttons naturally align their content to the left because they're modeled on bullet points (Windows 7 Copy Dialog).

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

Yes I understand the alignment, but the button border should shrink to the size of the button text of the largest width. The shrunk version looks 10x better.

@Helios747
Copy link
Contributor

I really dislike that yellow stripe image at the top. As @MaJoRoesch said, it is way too severe.

It's a UI for a video game console emulator, not a nuclear missile control system.

@Parlane is a troll. :P

@slx7R4GDZM
Copy link
Contributor

It'd probably look way better with some sort of red background instead of the yellow/black stripes.

@ghost
Copy link

ghost commented Jul 13, 2016

I think a more standard looking warning dialog would be a better choice. I also agree the yellow stripe is too over the top.

Something like this might be good:

DolphinQt Warning

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

OK @pringo's design is definitely better IMO.

Add a GUI prompt to tell users not to use DolphinQt by accident.
@EmptyChaos
Copy link
Contributor Author

@Helios747 I was thinking "construction site" rather than "nuclear silo".

@slx7R4GDZM The pattern and colors were based on hazard stripes, e.g. Barricade Tape

@pringo I assume that was a mockup? I couldn't figure out how to extend QMessageBox sanely so I just reimplemented the design manually.

experimentalaero2

@mimimi085181
Copy link
Contributor

What about a checkbox: "I know what i'm doing, stop asking", that appears after you launched QT for a few times, and which prevents showing the screen in the future?

@phire
Copy link
Member

phire commented Jul 13, 2016

There is a ini file setting you can set to make the warning go away. I'm not sure we want to expose that setting to a GUI just yet, maybe when DolphinQt gets to a less experimental state.

@Parlane
Copy link
Member

Parlane commented Jul 13, 2016

LGTM.

@phire
Copy link
Member

phire commented Jul 13, 2016

:lgtm:


Reviewed 1 of 9 files at r1, 1 of 9 files at r2, 6 of 8 files at r3, 1 of 2 files at r4, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@MayImilae
Copy link
Contributor

Much better! LGTM

@phire phire merged commit cb63bac into dolphin-emu:master Jul 13, 2016
@EmptyChaos EmptyChaos deleted the dqt2-warning branch July 13, 2016 09:20
@ghost
Copy link

ghost commented Jul 13, 2016

@EmptyChaos Yeah it was just a mockup.

@mbc07
Copy link
Contributor

mbc07 commented Jul 15, 2016

I think it's worth mentioning the dialog seems to not play nice with high DPI settings, the last lines of text are cropped on those scenarios:
capturar

@ghost
Copy link

ghost commented Jul 15, 2016

@mbc07 is that from before or after #3737 was merged?

@Nucleoprotein
Copy link

@pringo
That dialog uses QT5 so has nothing to do with #3737.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
10 participants