Add "Show intro page" button to System Settings sidebar
AbandonedPublic

Authored by GB_2 on Aug 19 2019, 8:18 PM.

Details

Summary

BUG: 405956

Test Plan

Open System Settings and click on the "Show intro page" button.

Diff Detail

Repository
R124 System Settings
Branch
add-show-intro-page-button-to-system-settings-sidebar (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16980
Build 16998: arc lint + arc unit
GB_2 created this revision.Aug 19 2019, 8:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 19 2019, 8:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
GB_2 requested review of this revision.Aug 19 2019, 8:18 PM
GB_2 added a subscriber: mart.Aug 19 2019, 8:30 PM

@mart can you help here?

mart added a comment.Aug 23 2019, 1:36 PM

when the intro page gets shown, (again binded from a bool property arriving from c++)
categoryView.currentIndex should be set to -1,
and systemsettings.activeCategory to -1 as well

sidebar/SidebarMode.cpp
426

this should be a bool property setter

sidebar/package/contents/ui/CategoriesPage.qml
43–44

would be nice to have this disabled when on intro page.
the systemsettings context property should expose a bool property true when the intro page is shown

72

I would prefer overflow-menu

mart added a comment.Aug 23 2019, 1:37 PM

there *may* be some changes needing to be done on SidebarMode::setActiveCategory for that to work, but try to do that, then wel'll see

ngraham added inline comments.
sidebar/package/contents/ui/CategoriesPage.qml
72

This is just moving the existing item around, which already uses the hamburger menu icon (appropriately IMO).

GB_2 updated this revision to Diff 64682.Aug 26 2019, 5:00 PM

Use a bool property setter and disable button when on intro page

GB_2 marked 2 inline comments as done.Aug 26 2019, 5:01 PM

This is what happens when you set the current index to -1:

mart added a comment.Aug 27 2019, 1:12 PM
In D23274#519661, @GB_2 wrote:

This is what happens when you set the current index to -1:

did you also try to set the active category to -1?

mart added a comment.Aug 27 2019, 1:12 PM

SidebarMode::setActiveCategory may need to be modified to make it like it tough

GB_2 added a comment.Aug 27 2019, 2:09 PM
In D23274#520127, @mart wrote:

SidebarMode::setActiveCategory may need to be modified to make it like it tough

Looks like it.

sidebar/SidebarMode.cpp
505

Yes, here.

mart added a comment.Aug 27 2019, 3:45 PM
In D23274#520185, @GB_2 wrote:
In D23274#520127, @mart wrote:

SidebarMode::setActiveCategory may need to be modified to make it like it tough

Looks like it.

can you take a look? should be fairly simple
const int newCategoryRow = to have -1 when the passed cat is -1
also, SidebarMode::changeModule to get special casing for invalid qmodelindexes

mart added a comment.Sep 3 2019, 10:23 AM

any updates on this?

GB_2 planned changes to this revision.Sep 8 2019, 1:21 PM
This comment was removed by GB_2.
GB_2 added a comment.Sep 23 2019, 4:52 PM

Looks like this is more complicated than I thought. If someone wants to comandeer this revision then please do.

Try this patch to your patch:

1diff --git a/sidebar/SidebarMode.cpp b/sidebar/SidebarMode.cpp
2index 3340d6f..8770189 100644
3--- a/sidebar/SidebarMode.cpp
4+++ b/sidebar/SidebarMode.cpp
5@@ -426,6 +426,10 @@ void SidebarMode::changeModule( const QModelIndex& activeModule )
6 {
7 d->moduleView->closeModules();
8
9+ if (!activeModule.isValid()) {
10+ return;
11+ }
12+
13 const int subRows = d->searchModel->rowCount(activeModule);
14 if ( subRows < 2) {
15 d->moduleView->loadModule( activeModule );
16@@ -449,13 +453,15 @@ int SidebarMode::activeCategory() const
17
18 void SidebarMode::setActiveCategory(int cat)
19 {
20+ const QModelIndex idx = d->searchModel->index(cat, 0);
21+ int newCategoryRow;
22 if (cat != -1) {
23 setIntroPageVisible(false);
24+ newCategoryRow = d->searchModel->mapToSource(idx).row();
25+ } else {
26+ newCategoryRow = cat;
27 }
28
29- const QModelIndex idx = d->searchModel->index(cat, 0);
30- const int newCategoryRow = d->searchModel->mapToSource(idx).row();
31-
32 if (d->activeCategory == newCategoryRow) {
33 return;
34 }

GB_2 updated this revision to Diff 66780.Sep 24 2019, 5:24 PM

Use Nate's fix and enable button when clicking a most used item

GB_2 added a comment.Sep 24 2019, 5:26 PM

Try this patch to your patch:

1diff --git a/sidebar/SidebarMode.cpp b/sidebar/SidebarMode.cpp
2index 3340d6f..8770189 100644
3--- a/sidebar/SidebarMode.cpp
4+++ b/sidebar/SidebarMode.cpp
5@@ -426,6 +426,10 @@ void SidebarMode::changeModule( const QModelIndex& activeModule )
6 {
7 d->moduleView->closeModules();
8
9+ if (!activeModule.isValid()) {
10+ return;
11+ }
12+
13 const int subRows = d->searchModel->rowCount(activeModule);
14 if ( subRows < 2) {
15 d->moduleView->loadModule( activeModule );
16@@ -449,13 +453,15 @@ int SidebarMode::activeCategory() const
17
18 void SidebarMode::setActiveCategory(int cat)
19 {
20+ const QModelIndex idx = d->searchModel->index(cat, 0);
21+ int newCategoryRow;
22 if (cat != -1) {
23 setIntroPageVisible(false);
24+ newCategoryRow = d->searchModel->mapToSource(idx).row();
25+ } else {
26+ newCategoryRow = cat;
27 }
28
29- const QModelIndex idx = d->searchModel->index(cat, 0);
30- const int newCategoryRow = d->searchModel->mapToSource(idx).row();
31-
32 if (d->activeCategory == newCategoryRow) {
33 return;
34 }

Thanks! The only issue that's left now is that the subcategory doesn't get reset (when going into for example Application Style and then pressing the button).

Yep, I figured you could figure that part out. :)

mart updated this revision to Diff 66806.Sep 25 2019, 11:10 AM
  • correctly hide subcat column
ngraham accepted this revision.Sep 25 2019, 1:42 PM

LGTM!

This revision is now accepted and ready to land.Sep 25 2019, 1:42 PM
GB_2 retitled this revision from [WIP] Add "Show intro page" button to System Settings sidebar to Add "Show intro page" button to System Settings sidebar.Sep 25 2019, 2:17 PM
GB_2 edited the summary of this revision. (Show Details)
GB_2 added a comment.EditedSep 25 2019, 2:20 PM

Shouldn't the "Show intro page" button be on the left, like in Discover, and the hamburger menu button on the right, like in for example Dolphin? (Goal: Consistency)

Yeah I would say so.

GB_2 updated this revision to Diff 66829.Sep 25 2019, 2:30 PM

Swap menu and home button position

This revision was automatically updated to reflect the committed changes.
mart added a comment.Sep 26 2019, 5:50 PM
In D23274#537632, @GB_2 wrote:

Shouldn't the "Show intro page" button be on the left, like in Discover, and the hamburger menu button on the right, like in for example Dolphin? (Goal: Consistency)

i really don't like that btw

mart reopened this revision.Sep 26 2019, 5:51 PM

Yeah I would say so.

i strongly disagree, it's the opposite of every other kirigami app, and i really want to change it back.

This revision is now accepted and ready to land.Sep 26 2019, 5:51 PM
mart requested changes to this revision.Sep 26 2019, 5:51 PM
This revision now requires changes to proceed.Sep 26 2019, 5:51 PM
In D23274#538315, @mart wrote:

Yeah I would say so.

i strongly disagree, it's the opposite of every other kirigami app, and i really want to change it back.

But then Kirigami is the opposite of other apps that have their hamburger menus on the right (Dolphin, Firefox, GNOME apps). We need to sort this out.

mart added a comment.Sep 26 2019, 6:21 PM
In D23274#538315, @mart wrote:

Yeah I would say so.

i strongly disagree, it's the opposite of every other kirigami app, and i really want to change it back.

But then Kirigami is the opposite of other apps that have their hamburger menus on the right (Dolphin, Firefox, GNOME apps). We need to sort this out.

I don't think it's an absolute rule for every app, but systemsettings uses the column layout with drill down and toolbars contextual only to the column they are on top of.
For pages toolbars of applications that use column based navigation, action buttons are on the right, title or a widget that replaces the title (like a search field) is on the left (which, is pretty much the android layout, or chat apps like whatsapp and telegram on any platform)
I would be hurted already a bit less from that systemsettings layout if that menu button was a ... icon instead, which is then commonly used by such apps.
For those column based layouts with column-specific toolbars i kinda see an "app-global" menu icon only on the left, i don't think it would ever work on the right at all.
Also because the pattern is from left column: more generic stuff, and as goes right, more drilled down, narrow-topic content

Dolphin toolbar i see it more like a different beast, a "global" toolbar that doesn't have anything to do with those context specific ones and has different rules.
There, is true that probably the menu button in front of the back button would be kinda strange, especially because the most used button is probably indeed, the back button and the menu button just once in a blue moon. (tough for dolphin as well, i think i would prefer a lot the ... as chromium does)

I could get behind always having an "app-global" hamburger menu on the left when the menubar isn't shown, but this would require implementing the same pattern for every app, by putting a little mini-toolbar above the app's sidebar that's separate from the toolbar above the view, which would then contain only view-specific actions. This is becoming pretty common and Kirigami apps can do this easily but our QWidgets apps can't do it as easily. Maybe it's time to look into that.

Either way I guess I can get on board with having the hamburger menu on the left here. We'll also need to do the same thing for Discover then.

mart added a comment.EditedSep 26 2019, 7:19 PM

I could get behind always having an "app-global" hamburger menu on the left when the menubar isn't shown, but this would require implementing the same pattern for every app, by putting a little mini-toolbar above the app's sidebar that's separate from the toolbar above the view, which would then contain only view-specific actions. This is becoming pretty common and Kirigami apps can do this easily but our QWidgets apps can't do it as easily. Maybe it's time to look into that.

To me we should look more into the differences of global vs page specific toolbars
I feel that the 2 different types of toolbars in this screenshot (whatsapp,telegram and kirigami gallery vs libreoffice and dolphin)
both make sense to exist and coexist (even in the same app at once if necessary) and they call for different rules and behavior


and for the global menu case, an hamburgher menu at the top right makes sense (tough to me could well be the ... icon as is kinda "actions that didn't fit the global toolbar")

I would be ok if it was wound a way that visually and conceptually works to move the hamburger of column view apps elsewhere, can't think of any

Either way I guess I can get on board with having the hamburger menu on the left here. We'll also need to do the same thing for Discover then.

Discover doesn't have any global menu i think? (iirc only an home button?) in the case of discover the "global thing" that kinda replaces the menubar becomes the sidebar when small or mobile

GB_2 added a comment.Sep 28 2019, 8:20 AM

This can be closed.

GB_2 added a comment.Sep 29 2019, 11:51 AM

@mart I found a bug with this: the titlebar title doesn't update.