Feature/start with last opened tabs #55

PoisonousJohn · 2017-05-03T13:00:19Z

Added "Continue where you left off" setting to the settings page.
Added "Session" entity to save session-related data to separate file
Added list of tab states to session.
Implemented restoration of tabs when opening first browser window

PoisonousJohn · 2017-05-03T16:19:59Z

This is for #50

timsueberkrueb · 2017-05-03T18:45:10Z

@JohnPoison thank you! I will hopefully be able to look into it tomorrow (or on Friday).

PoisonousJohn · 2017-05-05T09:30:52Z

Also I'm currently looking into saving tab's scrolling progress upon exit. It seems that for this task we'll have to use WebChannel. Also this will give us more flexible way to communicate QML <--> WebPage content

timsueberkrueb · 2017-05-05T13:57:51Z

@JohnPoison That would be cool :) If we have to use WebChannels, we should merge feature/qtwebengine first. But couldn't we just get the scroll position by running some JavaScript in the page as we do for theme color support?

PoisonousJohn · 2017-05-05T14:01:24Z

@tim-sueberkrueb sure we can. But isn't that lot easier to modify property of a js object, instead of messing with runJavaScript callbacks? Besides it's not so easy to use runJavaScript callback, since I'm saving sesison data on window close. So I should block closing until callback returns.

I see that web channel can give us a handy way to communicate with web page content. Well, we can even do this in a separate PR

timsueberkrueb · 2017-05-05T14:02:32Z

@JohnPoison I see, that's right. We could also use WebChannels for theme color handling then.

timsueberkrueb · 2017-05-07T10:33:34Z

feature/qtwebengine was merged with develop (including the travis fix). You'll probably want to rebase your work on develop. Thanks!

PoisonousJohn · 2017-05-08T19:41:01Z

Lol, it seems i've messed up with git. Will fix it 10th of May.

PoisonousJohn · 2017-05-10T11:50:51Z

@tim-sueberkrueb rebase done.

timsueberkrueb

Hey, I'm so sorry that I forgot about this PR ._.
Can you rebase it on develop?
Looks good otherwise 👍
We might want to rethink settings (in general) though (but that's something for v0.5.0 ;)).

timsueberkrueb · 2017-08-14T09:47:13Z

src/ui/tabview/content/SettingsContent.qml

@@ -61,11 +61,35 @@ TabContent {
                }

                TitleLabel {
-                    text: "Start"
+                    text: "Startup"


Please use qsTr() now

timsueberkrueb · 2017-08-14T09:47:43Z

src/ui/tabview/content/SettingsContent.qml

+                }
+
+                Column {
+                    Label { text: "Start new window with" }


Please use qsTr() now

timsueberkrueb · 2017-08-14T09:54:19Z

src/core/session/session.cpp

+ * $END_LICENSE$
+*/
+
+#include "Session.h"


I think that's "session.h".

timsueberkrueb · 2017-08-14T11:08:48Z

I think one thing to consider is whether it would be better if restored tabs would only load on demand (e.g. when the user activates them) as it is the case in Firefox. That would prevent long loading times when many tabs are stored.
What do you think?

ShalokShalom · 2017-08-14T18:51:05Z

Completely agree. +1

Just think about cases with 20-40 tabs open and more..

timsueberkrueb · 2017-09-30T20:04:48Z

Another problem with this I haven't thought of yet is multi window support.

PoisonousJohn · 2018-03-14T09:24:26Z

Rebased on develop.
Added lazy loading of the tabs.

Regarding multi-window support, right now the session is being saved for the last closed window. I think if we need multi-window support, it should be done as a separate issue.

timsueberkrueb

Thanks a lot for getting back to this!
Looking good, will check it out and test myself asap.

timsueberkrueb · 2018-03-14T18:35:24Z

src/ui/tabview/TabController.qml

+    {
+        for (var i = 0; i < tabContentView.pages.count; ++i)
+        {
+            var tabPage = tabContentView.pages.get(i).item;


Let's stay consistent with the rest of the JavaScript code and keep the opening curly brace in the same line ;)

for (var i = 0; i < tabContentView.pages.count; ++i) {

timsueberkrueb · 2018-03-14T18:36:24Z

src/ui/tabview/TabController.qml

-            background: background
-        });
+    function completeTabsRestore()
+    {


Let's keep the opening curly brace in the same line for consistency ;)

function completeTabsRestore() {

timsueberkrueb · 2018-03-14T18:38:28Z

src/ui/tabview/TabPage.qml

-                );
-                break;
+        if (!loadContent)
+        {


Let's keep the opening curly brace in the same line for consistency.

timsueberkrueb · 2018-03-14T18:38:43Z

src/ui/tabview/TabPage.qml

        }
+        else
+        {


Let's keep the opening curly brace in the same line for consistency.

timsueberkrueb · 2018-03-14T18:39:28Z

src/ui/tabview/content/LazyWebContent.qml

+        id: lazyCreateConnection
+
+        function tryCreateContent()
+        {


Let's keep the opening curly brace in the same line for consistency.

timsueberkrueb · 2018-03-14T18:41:06Z

src/ui/tabview/content/LazyWebContent.qml

+                return;
+
+            if (parentTabPage.loadContent)
+            {


Let's keep the opening curly brace in the same line for consistency.

timsueberkrueb · 2018-03-14T18:41:42Z

src/core/session/session.cpp

+
+    m_activeTab = std::max(m_tabs.count() - 1, 0);
+    if (root.find("activeTab") != root.end())
+    {


Let's keep the opening curly brace in the same line for consistency-

sorry for that braces, my old habit from c# :)

No problem, thanks! :)

timsueberkrueb · 2018-03-14T18:47:17Z

src/core/session/session.h

+    Q_PROPERTY(int activeTab READ getActiveTab)
+
+    int getActiveTab() { return m_activeTab; }
+signals:


We can remove the signals and public slots sections as they are not used.

timsueberkrueb

Looks like the data directory (/home/tim/.local/share/liri-browser in my case) is not being created:
Couldn't open session file for write!

We should make sure the directory gets created, see https://github.com/lirios/browser/blob/develop/src/core/settings/settings.cpp#L40

timsueberkrueb · 2018-03-14T19:36:01Z

src/core/session/tabstate.cpp

+
+float TabState::readingProgress() const
+{
+    return m_readingProgress;


Should we remove readingProgress while it's not being used, yet?

timsueberkrueb · 2018-03-14T19:40:19Z

src/core/session/session.cpp

+        tabObject["url"] = tab->url().toString();
+        tabObject["title"] = tab->title();
+        tabObject["icon"] = tab->iconUrl().toString().replace("image://favicon/","");
+        tabObject["readingProgress"] = 0.f;


The JSON fields for settings are currently all snake_case. I do prefer camelCase though. We can change the settings schema to use camelCase (which shouldn't be a problem right now as we don't have a release yet).
Any objections?

No objections. Should I leave it like that for now?

Yep, let's :)

PoisonousJohn · 2018-03-15T07:50:44Z

Thinking about it, couldn't we merge LazyWebContent and WebContent? I'm thinking about making WebContent just contain the loader wrapping the WebEngineView and getting an additional lazy property to toggle default or lazy loading.

I have thought about it, and even tried, but something went wrong. Let me try again.
Then it should fix problems with the settings page as well

PoisonousJohn · 2018-03-15T07:55:49Z

Looks like the data directory (/home/tim/.local/share/liri-browser in my case) is not being created:
Couldn't open session file for write!
We should make sure the directory gets created, see https://github.com/lirios/browser/blob/develop/src/core/settings/settings.cpp#L40

I almost sure that I have fixed it. It seems I've lost the change during problems with rebase. Let me fix it again

ShalokShalom · 2018-03-15T08:32:25Z

Oh rebase ^-^

ShalokShalom · 2018-03-15T08:33:43Z

https://hackernoon.com/dont-fear-the-rebase-bca683888dae

PoisonousJohn · 2018-03-15T09:43:02Z

This iteration looks more appropriate. liri-settings bug is fixed as well

timsueberkrueb · 2018-03-15T17:11:04Z

Awesome, thank you! Will check it out a bit later today!

timsueberkrueb

Works great now, thank you! One thing I noticed is that you store the icon url but the icons will only show up when the tab fully loads. We either should make the icons load or drop the property.
We also probably want to prevent saving the icon url for liri:// tabs:

"icon": "qrc:/liri.io/imports/Fluid/Controls/icons/action/settings.svg",

It also looks like the icon information gets lost when I restart the browser, tabs get restored but I don't activate them and restart the browser again:

"icon": ""

Besides of that, looking good!

timsueberkrueb · 2018-03-15T19:38:04Z

src/ui/tabview/TabPage.qml

@@ -59,6 +55,15 @@ Item {
    property var contentItem
    property int tabType

+    function tryCreateContent() {
+        console.log("Try create content");


Do you want to keep this logging?

timsueberkrueb · 2018-03-15T19:39:56Z

src/core/session/session.cpp

@@ -42,6 +42,11 @@ Session::Session(QObject *parent)

 void Session::save(TabsModel* tabs)
 {
+    if (!QDir(Paths::DataLocation).exists()) {
+        qDebug() << "DataLocation path doesn't exist.";


Let's make this a bit more human-friendly, like "App data path doesn't exist".

timsueberkrueb · 2018-03-15T19:42:53Z

src/ui/tabview/TabPage.qml


        return true;
    }

+    onActiveTabChanged: {
+        console.log("onActiveTabChanged");


Can we remove this debug statement?

timsueberkrueb · 2018-03-15T19:49:35Z

src/core/session/session.cpp

+{
+    QVariantList tabs;
+    for (TabState* state : m_tabs)
+    {


Found another curly brace :)

timsueberkrueb · 2018-03-15T19:50:53Z

src/ui/tabview/TabPage.qml

@@ -27,11 +27,16 @@ import ".."
 Item {
    id: tabPage

+    // binding. should be set from outside


Is this comment still relevant? I don't understand what it means.

It's relevant. It means that activeTab property itself doesn't do anything. It should be a functions assigned by another component. This function should return an activeTab. I did this to avoid dependency on TabController

Ah, all right.

timsueberkrueb · 2018-03-15T19:51:27Z

src/ui/tabview/TabPage.qml

+        if (contentItem || !loadContent || tab !== activeTab)
+            return;
+
+        console.log("Creating content");


Do we need this debug statement?

timsueberkrueb · 2018-03-15T20:12:06Z

src/core/session/session.cpp

+        tabObject["url"] = tab->url().toString();
+        tabObject["title"] = tab->title();
+        tabObject["icon"] = tab->iconUrl().toString().replace("image://favicon/","");
+        tabObject["readingProgress"] = 0.f;


readingProgress can be removed here, right?

PoisonousJohn · 2018-03-16T14:23:34Z

We also probably want to prevent saving the icon url for liri://
I would leave it as is for consistency, since it works fine. I don't want to check url for "liri://".

PoisonousJohn · 2018-03-16T14:26:38Z

src/ui/window/BrowserWindow.qml

@@ -502,7 +502,7 @@ ApplicationWindow {
                    'loadContent': false,
                    'url': restoreTabs[i].url,
                    'title': restoreTabs[i].title,
-                    'iconUrl': restoreTabs[i].iconUrl
+                    'iconUrl': restoreTabs[i].icon.replace("https", "http")


This bothers me. Right now QML Image is not able to load images via https, showing SSL error in console. Yet browser works fine with SSL. Don't know how to fix it, so it's a hack right now.

Interesting, I cannot reproduce this issue here on Ubuntu 17.10 with Qt 5.10.1.

timsueberkrueb · 2018-03-16T17:50:46Z

src/ui/tabview/content/SettingsContent.qml

-                Label {
-                    text: qsTr("Start url")
+                Column {
+                    Label { text: "Start new window with" }


Use qsTr()

timsueberkrueb · 2018-03-16T17:56:53Z

Looking good otherwise!

PoisonousJohn · 2018-03-21T18:58:51Z

@tim-sueberkrueb I believe everything is ok now :)

timsueberkrueb · 2018-03-21T20:12:35Z

@PoisonousJohn I think you need to remove replace("https", "http") :)

PoisonousJohn · 2018-03-21T20:14:06Z

@tim-sueberkrueb yeah, sure, I forgot about that, still didn't test it in a "Empty" project

PoisonousJohn · 2018-03-21T20:17:25Z

@tim-sueberkrueb done

timsueberkrueb · 2018-03-21T21:16:50Z

@PoisonousJohn do you have any preference on merging the PR? Should I squash your commits?

timsueberkrueb · 2018-03-21T21:31:58Z

Thank you @PoisonousJohn!

PoisonousJohn requested a review from timsueberkrueb May 3, 2017 13:00

PoisonousJohn force-pushed the feature/start_with_last_opened_tabs branch from dd8fecd to 27c617f Compare May 8, 2017 18:56

PoisonousJohn force-pushed the feature/start_with_last_opened_tabs branch 2 times, most recently from 5177adb to a88b184 Compare May 10, 2017 11:47

timsueberkrueb requested changes Aug 14, 2017

View reviewed changes

timsueberkrueb reviewed Aug 14, 2017

View reviewed changes

src/core/session/session.cpp Outdated

* $END_LICENSE$

*/

#include "Session.h"

Copy link

Collaborator

timsueberkrueb Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's "session.h".

PoisonousJohn force-pushed the feature/start_with_last_opened_tabs branch 2 times, most recently from ecd37d8 to b2f5836 Compare March 14, 2018 09:19

Ivan Fateev and others added 6 commits March 14, 2018 14:34

Finished main work on lirios#50

2f7dbd0

Added missing copyrights header

5169e92

fixed sesson header

4c786df

Finished main work on lirios#50

6ef5bde

Added missing copyrights header

a187110

do not load all tabs upon restoring

b2f5836

timsueberkrueb reviewed Mar 14, 2018

View reviewed changes

Ivan Fateev added 3 commits March 15, 2018 12:39

fixes according to the review

c57252e

forgotten stylefix

ae27765

more stylefixes

aa3de28

timsueberkrueb requested changes Mar 15, 2018

View reviewed changes

timsueberkrueb reviewed Mar 15, 2018

View reviewed changes

PoisonousJohn added the feature label Mar 16, 2018

PoisonousJohn self-assigned this Mar 16, 2018

PoisonousJohn commented Mar 16, 2018

View reviewed changes

fixed loading of icons, few style fixes as well

a7d0768

timsueberkrueb reviewed Mar 16, 2018

View reviewed changes

added missing qsTr

370fa1b

removed https hack

2457b9d

timsueberkrueb approved these changes Mar 21, 2018

View reviewed changes

timsueberkrueb merged commit 2c13e35 into lirios:develop Mar 21, 2018

Feature/start with last opened tabs #55

Feature/start with last opened tabs #55

Conversation

PoisonousJohn commented May 3, 2017 • edited Loading

PoisonousJohn commented May 3, 2017

timsueberkrueb commented May 3, 2017

PoisonousJohn commented May 5, 2017

timsueberkrueb commented May 5, 2017

PoisonousJohn commented May 5, 2017

timsueberkrueb commented May 5, 2017

timsueberkrueb commented May 7, 2017

PoisonousJohn commented May 8, 2017

PoisonousJohn commented May 10, 2017

timsueberkrueb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsueberkrueb commented Aug 14, 2017

ShalokShalom commented Aug 14, 2017

timsueberkrueb commented Sep 30, 2017 • edited Loading

PoisonousJohn commented Mar 14, 2018

timsueberkrueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsueberkrueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoisonousJohn commented Mar 15, 2018

PoisonousJohn commented Mar 15, 2018

ShalokShalom commented Mar 15, 2018

ShalokShalom commented Mar 15, 2018

PoisonousJohn commented Mar 15, 2018

timsueberkrueb commented Mar 15, 2018

timsueberkrueb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoisonousJohn commented Mar 16, 2018

PoisonousJohn Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsueberkrueb commented Mar 16, 2018

PoisonousJohn commented Mar 21, 2018

timsueberkrueb commented Mar 21, 2018

PoisonousJohn commented Mar 21, 2018

PoisonousJohn commented Mar 21, 2018

timsueberkrueb commented Mar 21, 2018

timsueberkrueb commented Mar 21, 2018

PoisonousJohn commented May 3, 2017 •

edited

Loading

timsueberkrueb left a comment •

edited

Loading

timsueberkrueb commented Sep 30, 2017 •

edited

Loading

PoisonousJohn Mar 16, 2018 •

edited

Loading