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.
Details
- Reviewers
rkflx - Group Reviewers
Gwenview - Commits
- R260:602db1bcbe08: Adjust sidebar code and config entries to statusbar
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
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. |
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.
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 {. |
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. |
app/sidebar.cpp | ||
---|---|---|
245 | I would rather keep it in SideBar. What do you think about e.g. SideBar::enableAutoSaveConfig()? |
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? |
app/sidebar.cpp | ||
---|---|---|
245 | Ah, wait a moment. Just found out about https://doc.qt.io/qt-5/qobject.html#blockSignals. Would this help? |
app/mainwindow.cpp | ||
---|---|---|
1297 | Your patch solved https://bugs.kde.org/show_bug.cgi?id=389558 too. 👍 |