Fix #186: Introduced Tab Restoration on Restart #1048

ShaopengLin · 2024-03-07T05:13:39Z

Changes:

~~Pop-up to prompt users whether they want to restore the tabs. If not, start a new session.~~ Added a checkbox in settings for users to toggle whether they want tab restoration or not.
Tab order is preserved.
Tab closures are recorded.
Tab navigation URL is preserved.
Blank/Settings tabs are recorded but not opened again on startup.

Fix-up Changes:

Does not open zim files that are deleted from the File System.
Pop-ups replaced with toggle option in settings.
Writes to a separate file than the program settings registry.
Simplifies session commit strategy.

kelson42 · 2024-03-08T19:44:24Z

@ShaopengLin Thank you very much for PR. I will check from a user perspective. One thing which should probably be changed is creating a dedicated settings to manage this behaviour (not in favour of popup at start).

ShaopengLin · 2024-03-08T19:49:00Z

Like an option for either restoring automatucally or have a pop-up for it? For automatic restoration it would be good to have a short-small notification about it as well.

kelson42 · 2024-03-08T20:10:18Z

Yes, a basic boolean setting "Restore tabs at start". I will lokk how FF and Chrome exactly work...

ShaopengLin · 2024-03-10T03:25:23Z

Just found we have a settingsmanager.cpp. Will move code over there to refactor a commit.

kelson42 · 2024-03-10T16:31:00Z

Yes, a basic boolean setting "Restore tabs at start". I will lokk how FF and Chrome exactly work...

I confirm, we should have a "Open previous tabs at startup" checkbox in the settings. Per default unchecked.

ShaopengLin · 2024-03-10T16:32:38Z

Yes, a basic boolean setting "Restore tabs at start". I will lokk how FF and Chrome exactly work...

I confirm, we should have a "Open previous tabs at startup" checkbox in the settings. Per default unchecked.

I think currently I set it as default checked, simple change. Will wait for reviewer comment and change in a batch.

kelson42 · 2024-03-10T16:49:13Z

@ShaopengLin Option not visible in the settings,

ShaopengLin · 2024-03-10T16:53:19Z

Weird. I will test it using Qt6 as well. I was still developing on the Qt5 build. Might be some differences in building UI?

kelson42 · 2024-03-10T17:04:14Z

@ShaopengLin Sorry, me doing a wrong manipulation. Seems to work fine... but setting wording is NOK. SHould be "Open previous tabs at startup"

ShaopengLin · 2024-03-10T17:50:06Z

Is there a way for me to update i18n for the text?

kelson42 · 2024-03-10T18:15:41Z

@ShaopengLin You should put only the string in English

ShaopengLin · 2024-03-10T18:41:05Z

@kelson42 I just put it like this?
ui->reopenTabLabel->setText(gt("open-previous-tabs-on-startup"));

src/settingsmanager.h

src/tabbar.cpp

ShaopengLin · 2024-03-12T17:51:29Z

I will update this PR after merge of #1057 and on resolution of the comments: comment on text translation and comment on session

src/kiwixapp.cpp

veloman-yunkan

I am a little concerned with the new calls to Kiwix::instance(), but I think that I'd rather address it separately in a dedicated design improvement PR.

Apart from fixing the issues outlined in the review comments, please split this PR in two commits:

Add unconditional reopening of tabs from previous session;
Introduce a setting that controls that behaviour.

src/tabbar.cpp

src/kiwixapp.h

src/kiwixapp.cpp

ShaopengLin · 2024-03-24T15:43:55Z

@veloman-yunkan Could you tag me when you start the refactoring work? I am a GSOC student so seeing practical design patterns being applied is great learning for me as well. I am guessing something like dependency injection or complete functions.

ShaopengLin · 2024-03-24T22:00:56Z

src/webview.cpp

@@ -166,12 +166,13 @@ QWebEngineView* WebView::createWindow(QWebEnginePage::WebWindowType type)

 void WebView::onUrlChanged(const QUrl& url) {
    auto zimId = getZimIdFromUrl(url);
+    auto app = KiwixApp::instance();
+    app->saveListOfOpenTabs();


I replaced the previous call on webpage.cpp as I found that was a bad choice. No need for the acceptNavigationRequest. onUrlChanged is indeed called on in-page URL navigations.

veloman-yunkan

In case the number of new comments makes you think that I am not happy with the progress you will be wrong. The fact is that we are much closer to having this PR merged and I am just paying attention to more mundane details. 😉

src/kiwixapp.cpp

veloman-yunkan · 2024-03-25T12:54:34Z

src/kiwixapp.cpp

+
+    /* Place session file in our global library path */
+    QDir dir(m_libraryDirectory);
+    this->mp_session = new QSettings(dir.filePath("user.session"),


IMHO, we better name kiwix-desktop's session file kiwix-desktop.session. @kelson42 Do you agree?

Made the modification just in case.

src/kiwixapp.cpp

src/tabbar.h

src/kiwixapp.cpp

veloman-yunkan · 2024-03-25T13:34:05Z

src/settingsview.cpp

-void SettingsView::init(int zoomPercent, const QString &downloadDir, const QString &monitorDir, const bool moveToTrash)
+void SettingsView::init(int zoomPercent, const QString &downloadDir,
+                        const QString &monitorDir, const bool moveToTrash,
+                        bool reopentab)


This is comment for myself. The trend of the parameter list of SettingsView::init() growing over time is obvious, but the need for that function is not. For a slightly different design the initialization of SettingsView could be performed via the existing update mechanism via signal-slot connections.

veloman-yunkan · 2024-03-25T13:48:54Z

@veloman-yunkan Could you tag me when you start the refactoring work? I am a GSOC student so seeing practical design patterns being applied is great learning for me as well. I am guessing something like dependency injection or complete functions.

🤝

I will. Can't promise that your expectations for design patterns will be met since I am mostly refactoring based on my sense of aesthetics, but if you notice in the resulting code anything familiar from your computer science study I will be glad to learn its name 😄

veloman-yunkan

Question to @kelson42 and @mgautierfr - in the current implementation the list of open tabs is saved every time it changes even if the "reopen-tabs" setting is disabled. Is that ok?

src/settingsview.cpp

resources/i18n/qqq.json

veloman-yunkan

LGTM (but note my so far unanswered question in the previous review).

mgautierfr · 2024-03-28T17:07:20Z

We may want to save (and restore) also the which tab is the current one.
But it may be done in another PR (open a issue in this case). Else nothing to add.

ShaopengLin · 2024-03-30T06:31:22Z

We may want to save (and restore) also the which tab is the current one. But it may be done in another PR (open a issue in this case). Else nothing to add.

~~@mgautierfr It's rather a simple change that makes this feature complete. I will just add it here. Sorry for the delayed response.~~

Actually no, it is not a 'simple' task and it would be better to have some discussion on the behaviors. I will open an issue once this is merged.

kelson42 · 2024-03-30T09:47:08Z

@ShaopengLin Thank you very much for this PR. Please don't forget to open the ticket like agreed. Merging...

Users can now restore tabs opened before close in the same order and same url. Fix kiwix#186

Added a checkbox in settings where user can define if they want the tab reopening feature to be active. Fix kiwix#186

The change in #1048 prevented opening of blank tab and urls that are no longer valid. Now, we will need to open them as well, as we will need the exact number of tabs to reopen in order to correctly restore current tab index.

ShaopengLin mentioned this pull request Mar 8, 2024

[Linux] Close Buttton on Other Tabs Closes the Displayed Tab Instead #1053

Closed

kelson42 requested a review from veloman-yunkan March 8, 2024 19:40

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch from 2a7f6ca to b1911ed Compare March 10, 2024 05:13

ShaopengLin mentioned this pull request Mar 11, 2024

Fix #1053: Close Button No Longer Only Close Current Tab on Linux #1054

Closed

veloman-yunkan reviewed Mar 11, 2024

View reviewed changes

src/settingsmanager.h Outdated Show resolved Hide resolved

src/tabbar.cpp Outdated Show resolved Hide resolved

mgautierfr reviewed Mar 21, 2024

View reviewed changes

src/kiwixapp.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch 2 times, most recently from fd80447 to 3f986b8 Compare March 22, 2024 17:58

ShaopengLin requested review from mgautierfr and veloman-yunkan March 22, 2024 18:00

veloman-yunkan requested changes Mar 24, 2024

View reviewed changes

src/tabbar.cpp Outdated Show resolved Hide resolved

src/tabbar.cpp Outdated Show resolved Hide resolved

src/tabbar.cpp Outdated Show resolved Hide resolved

src/tabbar.cpp Outdated Show resolved Hide resolved

src/kiwixapp.h Outdated Show resolved Hide resolved

veloman-yunkan reviewed Mar 24, 2024

View reviewed changes

src/kiwixapp.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch 2 times, most recently from 34edb31 to 2fae684 Compare March 24, 2024 21:56

ShaopengLin commented Mar 24, 2024

View reviewed changes

ShaopengLin requested a review from veloman-yunkan March 24, 2024 22:01

veloman-yunkan requested changes Mar 25, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch from 2fae684 to e66cfeb Compare March 27, 2024 02:11

ShaopengLin requested a review from veloman-yunkan March 27, 2024 02:13

veloman-yunkan requested changes Mar 27, 2024

View reviewed changes

src/settingsview.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch from e66cfeb to 9430e3f Compare March 27, 2024 15:15

ShaopengLin requested a review from veloman-yunkan March 27, 2024 15:16

veloman-yunkan reviewed Mar 27, 2024

View reviewed changes

resources/i18n/qqq.json Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#186-remember-tabs-opened branch from 9430e3f to f133b70 Compare March 27, 2024 18:22

ShaopengLin requested a review from veloman-yunkan March 28, 2024 06:51

veloman-yunkan approved these changes Mar 28, 2024

View reviewed changes

ShaopengLin added 2 commits March 30, 2024 10:47

Add Auto Reopening of Tabs from Previous Session

3235bda

Users can now restore tabs opened before close in the same order and same url. Fix kiwix#186

Add CheckBox in Settings for Tab Reopen Behavior

01a6698

Added a checkbox in settings where user can define if they want the tab reopening feature to be active. Fix kiwix#186

kelson42 force-pushed the Issue#186-remember-tabs-opened branch from f133b70 to 01a6698 Compare March 30, 2024 09:47

kelson42 merged commit 0205db4 into kiwix:main Mar 30, 2024
4 checks passed

ShaopengLin mentioned this pull request Mar 30, 2024

Remember current tab from previous user session #1073

Closed

veloman-yunkan mentioned this pull request Apr 11, 2024

Import file path memorization #1058

Merged

veloman-yunkan mentioned this pull request Jun 3, 2024

Remembers Previous Current Tab Index From Last Session #1120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #186: Introduced Tab Restoration on Restart #1048

Fix #186: Introduced Tab Restoration on Restart #1048

ShaopengLin commented Mar 7, 2024 •

edited

Loading

kelson42 commented Mar 8, 2024

ShaopengLin commented Mar 8, 2024

kelson42 commented Mar 8, 2024 •

edited

Loading

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024 •

edited

Loading

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024 •

edited

Loading

ShaopengLin commented Mar 12, 2024 •

edited

Loading

veloman-yunkan left a comment

ShaopengLin commented Mar 24, 2024

ShaopengLin Mar 24, 2024

veloman-yunkan left a comment

veloman-yunkan Mar 25, 2024

ShaopengLin Mar 27, 2024

veloman-yunkan Mar 25, 2024

veloman-yunkan commented Mar 25, 2024

veloman-yunkan left a comment

veloman-yunkan left a comment

mgautierfr commented Mar 28, 2024

ShaopengLin commented Mar 30, 2024 •

edited

Loading

kelson42 commented Mar 30, 2024

Fix #186: Introduced Tab Restoration on Restart #1048

Fix #186: Introduced Tab Restoration on Restart #1048

Conversation

ShaopengLin commented Mar 7, 2024 • edited Loading

kelson42 commented Mar 8, 2024

ShaopengLin commented Mar 8, 2024

kelson42 commented Mar 8, 2024 • edited Loading

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024 • edited Loading

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024

kelson42 commented Mar 10, 2024

ShaopengLin commented Mar 10, 2024 • edited Loading

ShaopengLin commented Mar 12, 2024 • edited Loading

veloman-yunkan left a comment

Choose a reason for hiding this comment

ShaopengLin commented Mar 24, 2024

ShaopengLin Mar 24, 2024

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Mar 25, 2024

Choose a reason for hiding this comment

ShaopengLin Mar 27, 2024

Choose a reason for hiding this comment

veloman-yunkan Mar 25, 2024

Choose a reason for hiding this comment

veloman-yunkan commented Mar 25, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Mar 28, 2024

ShaopengLin commented Mar 30, 2024 • edited Loading

kelson42 commented Mar 30, 2024

ShaopengLin commented Mar 7, 2024 •

edited

Loading

kelson42 commented Mar 8, 2024 •

edited

Loading

kelson42 commented Mar 10, 2024 •

edited

Loading

ShaopengLin commented Mar 10, 2024 •

edited

Loading

ShaopengLin commented Mar 12, 2024 •

edited

Loading

ShaopengLin commented Mar 30, 2024 •

edited

Loading