Adjust sidebar code and config entries to statusbar
ClosedPublic

Authored by muhlenpfordt on Feb 20 2018, 4:05 PM.

Details

Summary

Setup config entries and default values of sidebar states in
gwenview.kcfg and use the generated access functions like it
is done for statusbar in D10577.
From a user's sight the sidebar behaviour and state saving is
not changed.

Test Plan

Switch browse/view, normal/fullscreen modes.
The sidebar state should always restore to previous setting and
button on statusbar should correspond to this state.
Three different states are (re)stored:

  • Browse mode (always equal in normal/fullscreen mode)
  • View mode normal (windowed) mode
  • View mode fullscreen mode

Switch the different tabs on sidebar and check if the last
activated is restored on next restart.

Diff Detail

Repository
R260 Gwenview
Branch
sidebar-toggle
Lint
Lint OK
Unit
No Unit Test Coverage
muhlenpfordt requested review of this revision.Feb 20 2018, 4:05 PM
muhlenpfordt created this revision.

I've also created a kconf_update file to migrate sidebar and statusbar config entries. This should go to a separate diff I suppose?

app/mainwindow.cpp
440

Using the triggered() signal here (equal to KStandardAction::ShowStatusbar) prevents re-calling the slot from setChecked() inside MainWindow::toggleSideBar() and we don't need the SignalBlocker anymore.

rkflx added a subscriber: rkflx.Feb 20 2018, 4:15 PM

I've also created a kconf_update file to migrate sidebar and statusbar config entries. This should go to a separate diff I suppose?

Wow, that's great. Normally it's best to include those directly with the change, but as the statusbar one landed without, I guess a separate patch for both is okay too.

rkflx accepted this revision.Feb 21 2018, 11:03 PM

Thanks for tackling this, works as before and the code LGTM (mostly).

Good job with the annotations/explanations BTW, this helps so much when reviewing…

const, ? :, modernizations etc. – Perfect!

app/mainwindow.cpp
697

Just noticed that in D10577#inline-50448 I suggested save instead of set, somehow I missed this in the final check. If you like it better too, change it here and git push the corresponding change for the statusbar.

818–822

d->mSideBar everywhere makes me wonder whether this could be moved to, well, the SideBar. And GwenviewConfig:: is all over the codebase already anyway.

Also, your testplan missed to mention that part (it works, though ;)

1005–1006

I don't see this kind of check anywhere else, it's kind of over the top now…

1097

Add space before {.

This revision is now accepted and ready to land.Feb 21 2018, 11:03 PM
muhlenpfordt marked 4 inline comments as done.
muhlenpfordt edited the test plan for this revision. (Show Details)

Moved page (re)store to SideBar and some minor modifications

app/mainwindow.cpp
818–822

During the first addPage() the currentChanged signal is emitted, so it should not be connected before that (i.e. not in the SideBar constructor). Forgot to add a comment on that.
I moved the connect to SideBar::addPage() and added SideBar::loadConfig() to keep config handling together. Ok this way?

rkflx accepted this revision.Feb 22 2018, 1:27 PM
rkflx added inline comments.
app/mainwindow.cpp
1482

Good idea! I think this should stay.

app/sidebar.cpp
245

Uh oh. I did not think of that problem…

How about moving the connect to the end of setupContextManagerItems?

muhlenpfordt added inline comments.Feb 22 2018, 2:00 PM
app/sidebar.cpp
245

I would rather keep it in SideBar. What do you think about e.g. SideBar::enableAutoSaveConfig()?

rkflx added a comment.Feb 22 2018, 2:39 PM

Sometimes it's so difficult to find the "right" spot… Your suggestion LGTM now.

Also, would you like to handle https://phabricator.kde.org/D10745?

app/sidebar.cpp
245

Yeah, that would make SideBar easier to understand.

I assume you mean to put the connect in your new member function, and then call that after all sidebar pages have been added? If so, that plan sounds good.

"Enable (…) config" was not immediately clear to me, perhaps enableConfigSaving?

rkflx added inline comments.Feb 22 2018, 2:42 PM
app/sidebar.cpp
245

Ah, wait a moment. Just found out about https://doc.qt.io/qt-5/qobject.html#blockSignals. Would this help?

rkflx added a comment.Feb 22 2018, 3:01 PM

I knew it!

1diff --git a/app/mainwindow.cpp b/app/mainwindow.cpp
2index 44b69088..d004316d 100644
3--- a/app/mainwindow.cpp
4+++ b/app/mainwindow.cpp
5@@ -521,6 +521,10 @@ struct MainWindow::Private
6 FileOpsContextManagerItem* fileOpsItem = new FileOpsContextManagerItem(mContextManager, mThumbnailView, actionCollection, q);
7
8 // Fill sidebar
9+
10+ // <comment>
11+ SignalBlocker blocker(mSideBar->tabBar());
12+
13 SideBarPage* page;
14 page = new SideBarPage(i18n("Folders"));
15 page->setObjectName(QLatin1String("folders"));
16diff --git a/app/sidebar.cpp b/app/sidebar.cpp
17index 0faac954..16c400e6 100644
18--- a/app/sidebar.cpp
19+++ b/app/sidebar.cpp
20@@ -219,6 +219,10 @@ SideBar::SideBar(QWidget* parent)
21 tabBar()->setFocusPolicy(Qt::NoFocus);
22 setTabPosition(QTabWidget::South);
23 setElideMode(Qt::ElideRight);
24+
25+ connect(tabBar(), &QTabBar::currentChanged, [=]() {
26+ GwenviewConfig::setSideBarPage(currentPage());
27+ });
28 }
29
30 SideBar::~SideBar()
31@@ -234,13 +238,6 @@ QSize SideBar::sizeHint() const
32 void SideBar::addPage(SideBarPage* page)
33 {
34 addTab(page, page->title());
35-
36- if (count() == 1) {
37- // Don't connect before first addTab() since it emits currentChanged()
38- connect(tabBar(), &QTabBar::currentChanged, [=]() {
39- GwenviewConfig::setSideBarPage(currentPage());
40- });
41- }
42 }
43
44 QString SideBar::currentPage() const

muhlenpfordt marked 4 inline comments as done.

Moved connect to SideBar constructor and use SignalBlocker in addPage()

That's it.
I just moved it inside SideBar::addPage().

rkflx accepted this revision.Feb 22 2018, 4:27 PM

Perfect!

I just moved it inside SideBar::addPage().

How did I miss this…

This revision was automatically updated to reflect the committed changes.
rkflx added inline comments.Mar 8 2018, 9:10 PM
app/mainwindow.cpp
1297