hugolabe / Wike

Wikipedia Reader for the GNOME Desktop
https://hugolabe.github.io/Wike/
GNU General Public License v3.0
257 stars 33 forks source link

Add a 'Restore window' option #155

Closed aunetx closed 1 year ago

aunetx commented 1 year ago

This would fix #100.

The app is a little bit laggy for me during the first seconds when opening several pages, but nothing too bad; and I guess the trade-off is worth it! I tried to add the pages via GLib.idle_add too to see if it would increase performances, but it does not.

It also switches to the latest opened tab when opening them, so it effectively restores the last session nearly entirely.

aunetx commented 1 year ago

By the way, I could try to work on some kind of lazy-loading of the pages, where the app:

Implementing something like that would not be too hard, it would make sure that the app is not laggy when opening it, everything would display correctly (better that with this PR, because the name is not visible when the page is loading) and, with a sane number of tabs opened, everything is entirely loaded in under 10s. Would you like this behaviour?

aunetx commented 1 year ago

I just added some kind of lazy-loading, it works quite well -- at least there is no bug, no crash, and it is faster than without using it.

I limited the number of pages to lazy-load automatically to 15 (so that is the user has 100 pages opened, they do no clog up its memory); maybe you would prefer a smaller value? I think that the minimum should be 5, but 15 may be too much.

I also selected a 500ms delay between loads; this should be good enough for most users but you can change it too if you want.

One thing I'm not sure about though: I managed to hide the loading spinner and title when the pages are lazy-loaded, so that the whole process is transparent to the user. However, if the user has a slow internet connection, it means it cannot see that the page is still loading: maybe that's not ideal? The perfect solution would be to:

so that if the page is long to load, at least the user knows it is just its internet connection.

I will see what I can do about it.

aunetx commented 1 year ago

Well, I limited the maximum number of pages to 7 finally (should be a good trade-off), and I fixed the previous issue I mentioned: now, pages lazy-load transparently in the background, but if the user switches to one that still has not finished to load, the spinner appears.

I think this PR is in its final state, now it's up to you to choose whether or not you want to include it!

By the way, I just found out about WebViewSessionState, which would permit us to save the content of the page when closing Wike, and load it from disk when re-opening it... It would have been wonderful to include it in this PR, but unfortunately I don't have any experience with saving binary files to disk (or to gsettings). If you want me to try to work on this, I could; however the current solution seems good enough!

aunetx commented 1 year ago

Just for the information, WebViewSessionState does not seem to actually save the page -- just some infos about the state, but nothing more. So in the future it could be used to remember for example the scroll position etc, but that's not so important right now.

hugolabe commented 1 year ago

Hi @aunetx First of all, thank you for your work.

I've been testing it and it seems to work well. However, in my opinion it would be better to remove lazy loading.

I think the best option is that when you start the application only the active tab is loaded and the rest are only loaded when they are selected. That is, it would be similar to the behavior you describe in your second message but eliminating the fourth point (the lazy loading of 7 tabs).

Other applications like GNOME Web use this same approach and this way we would keep the code simpler (something that is important to me).

A detail. I think the text "Restore window" in the preferences dropdown is a bit confusing. Maybe something like "Restore all tabs" would be clearer.

aunetx commented 1 year ago

Thanks for your review! I just removed automatic lazy-loading, changed the label you wanted, and changed the way the first page is loaded too (that's the only option I found that did not make the first tab load every time, and that makes the application startup a little bit more sane in my opinion).

vanillajonathan commented 1 year ago

I think the option to configure this should be removed and that Wike should always restore the tabs.

Either that, or I think restore tabs should be the default behavior.

hugolabe commented 1 year ago

Sorry @vanillajonathan but I don't agree. It seems appropriate to me to be able to choose the behavior at startup and setting it as the default option does not seem to suit my approach to the app.