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

Qt/Configure: Use sidebar to divide tabs into smaller groups #4187

Merged
merged 1 commit into from Sep 14, 2018

Conversation

Projects
None yet
5 participants
@spycrab
Copy link
Contributor

spycrab commented Sep 6, 2018

Screenshot:
Screenshot

The specific grouping might still be suboptimal.


This change is Reviewable

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch from 1042961 to eee30b5 Sep 6, 2018


void ConfigureDialog::updateVisibleTabs() {
static const QHash<QString, QWidget*> m_widgets = {
{tr("General"), ui->generalTab}, {tr("System"), ui->systemTab},

This comment has been minimized.

Copy link
@lioncash

lioncash Sep 6, 2018

Member

This will likely break with translations, since the table will only ever be created once (and thus not retranslate). You may want to enclose the strings with QT_TR_NOOP(), so Linguist can still see these as translatable string, then call QObject::tr() on tab and use the resulting string to index this QHash in the loop below.

That, or have another means to do the lookup (or just make the QHash non-static, if the method isn't intended to be called frequently)

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch 2 times, most recently from ad761cd to 34b86bb Sep 6, 2018

@spycrab spycrab changed the title Qt/Configure: Use sidebar to devide tabs into smaller groups Qt/Configure: Use sidebar to divide tabs into smaller groups Sep 6, 2018

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch 2 times, most recently from d6b48eb to a72dbc9 Sep 6, 2018

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

about UX: the dialog is resized when you change selected items. For example, selecting "Input" would make the dialog much bigger and it won't restore even if you select other items. I wonder if it's possible to add a scroll bar and keep the dialog at the same size.

void ConfigureDialog::populateSelectionList() {
auto* general_item = new QListWidgetItem(tr("General"));

general_item->setData(Qt::UserRole, QStringList{tr("General"), tr("Web"), tr("Debug")});

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

Can you make this into an array as well and iterate through it initializing items, like what you did with the m_widgets below.

This comment has been minimized.

Copy link
@spycrab

spycrab Sep 8, 2018

Author Contributor

Can you clarify this?

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 8, 2018

Member

What I have in mind:

const std::array<std::pair<QString, QStringList>> items{{
    {tr("General"), QStringList{tr("General"), tr("Web"), tr("Debug")}},
   ...
}};

then you loop through it initializing items.
This would make it easier to change items.

if (items.isEmpty())
return;

const QHash<QString, QWidget*> m_widgets = {

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

This should not be prefixed with m_ as it is not a class member

@@ -21,6 +21,8 @@ class ConfigureDialog : public QDialog {
~ConfigureDialog();

void applyConfiguration();
void updateVisibleTabs();

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

function names should follow PascalCase.

Note: I know applyConfiguration already broke the convention, but new functions should still be in PascalCase

@@ -2,6 +2,9 @@
// Licensed under GPLv2 or any later version
// Refer to the license.txt file included.

#include <QHash>
#include <QListWidgetItem>

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 7, 2018

Member

We do not separate includes

This comment has been minimized.

Copy link
@spycrab

spycrab Sep 8, 2018

Author Contributor

Even when some are local and some are in a library / stdlib?

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 8, 2018

Member

no. we do not split them.

This comment has been minimized.

Copy link
@wwylele

wwylele Sep 8, 2018

Member

Explanation: splitting includes seemed to confuse clang-format and it wouldn't do proper sorting. Not sure if this still applies to current version.

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 8, 2018

Member

Yes, I can confirm that clang format won't properly sort all the includes when they are splitted. Instead it would sort the parts separately.

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch 3 times, most recently from 537693a to 1f3b446 Sep 8, 2018

@spycrab

This comment has been minimized.

Copy link
Contributor Author

spycrab commented Sep 8, 2018

@zhaowenlan1779 Size should stay the same now.

{tr("General"), tr("Web"), tr("Debug")}),
std::make_pair<QString, QStringList>(tr("System"), {tr("System"), tr("Audio")}),
std::make_pair<QString, QStringList>(tr("Graphics"), {tr("Graphics")}),
std::make_pair<QString, QStringList>(tr("Controls"), {tr("Input")})};

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Sep 9, 2018

Member

Will this code work as well?

const std::array<std::pair<QString, QStringList>, 4> items{{
    {tr("General"), {tr("General"), tr("Web"), tr("Debug")}},
    {tr("System"), {tr("System"), tr("Audio")}},
    {tr("Graphics"), {tr("Graphics")}},
    {tr("Controls"), {tr("Input")}}}};

Notice the double {}.

@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Sep 9, 2018

BTW, tested your latest commit's build, size still changes

@spycrab

This comment has been minimized.

Copy link
Contributor Author

spycrab commented Sep 9, 2018

Can't see that at all here:
GIF

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch from 1f3b446 to c02c8b5 Sep 9, 2018

@FearlessTobi

This comment has been minimized.

Copy link
Contributor

FearlessTobi commented Sep 9, 2018

Maybe it's a linux specific issue, like the cough volume slider?

@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Sep 10, 2018

Hmm, it's probably the geometry restore thing, so I'm not really sure now..

But anyway, the thing is still resized on my system that is Windows.
configure

@spycrab

This comment has been minimized.

Copy link
Contributor Author

spycrab commented Sep 11, 2018

Welp, I guess the size parameter given in the UI file was pretty much bad from the get go then.
I'll try to work around it.

Update:

Should be fixed now.

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch from c02c8b5 to fca9215 Sep 11, 2018

@spycrab spycrab force-pushed the spycrab:qt_config_sidebar branch from fca9215 to d63a63c Sep 11, 2018

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

Code LGTM.
Since the Input tab is so large, I think probably we can split it into a few tabs like Buttons, Sticks or something. But I believe it is something that needs to be discussed, and probably not really related to this PR.

@wwylele
Copy link
Member

wwylele left a comment

I feel uncomfortable with using UI string (which is translatable) as hash table key, but I guess as long as it works (judging by the video above), I am ok with it.

@wwylele wwylele merged commit 6767f45 into citra-emu:master Sep 14, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rcyggdra rcyggdra referenced this pull request Sep 15, 2018

Closed

Missing Camera option #4218

@spycrab spycrab deleted the spycrab:qt_config_sidebar branch Sep 15, 2018

@zhaowenlan1779 zhaowenlan1779 referenced this pull request Sep 15, 2018

Merged

Progress Report 2018 Q2 #91

4 of 4 tasks complete

chris062689 pushed a commit to yuzu-emu/yuzu-nightly that referenced this pull request Jan 2, 2019

Merge pull request #1944 from FearlessTobi/port-4187
Port citra-emu/citra#4187: "Qt/Configure: Use sidebar to divide tabs into smaller groups"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.