Hello all!


I would like to make a survey about which behavior of the plugin you would prefer, because I see three different ways of how a saved session can be loaded.


Version 1:
Let all existing windows untouched and open the saved session's windows in addition.

Version 2:
Close all existing windows when loading the saved session has been successful.
So the saved session being loaded would replace the existing one.

Version 3:
Ask the user with a simple Yes/No dialog with "Yes" as default:
Close all currently open windows?


Version 1 is implemented in the previously attached patch (https://bugzilla.gnome.org/attachment.cgi?id=147058).

Version 2 I've implemented, too, but not published yet. However it also works like a charm.

Version 3 is the one I personally  favor, although I've not yet implemented it. However that's only a matter of another few minutes of coding ;-)

Please help me to make a decision!


Thank you in forward.

Klaus

Personally I don't like version 3: we should try not to put choices in front of users, instead we should make stuff Just Work(tm). 
How about choosing option 2, but when existing windows were open, you save the existing windows to a temporary "Previous session"? That way you wouldn't lose data in case of a mistake because you could always go back.

Created attachment 147213 [details] [review]
Session Manager extension, version 3

Version 3:
Ask the user with a simple Yes/No dialog with "Yes" as default:
Close all currently open windows?

Thanks for your feedback!


> Personally I don't like version 3: we should try not to put choices in front of
> users, instead we should make stuff Just Work(tm). 

Of course I also like it when things Just Work(tm).

For myself, however I would prefer version 3 because it means only one mouseclick (or at maximum two  keypresses) in addition and there would be still be the "merge two sessions"-feature, i.e. one could choose not to close the windows currently open.
This last point is also the reason for me to prefer version 1 over version 2, because one can always close windows after loading a session anyway.

For the case you want to see and try, I've attached an additional patch for
version 3.

> How about choosing option 2, but when existing windows were open, you save the
> existing windows to a temporary "Previous session"? That way you wouldn't lose
> data in case of a mistake because you could always go back.

Yeah, I think that version 2 would be less user-friendly than both versions 1 and 3, if we don't introduce a "Load Previous Session…" function.

To conclude, I would like to put another version 4 into discussion and exclude version 2, as version 2 is probably the least user-friendly:


Version 4:
Like version 2, but with a "Load Previous Session…" function which allows to restore the previous session after loading a saved session. The according UI element ("Load Previous Session…") would be added after loading a saved session has completed successfully.

(In reply to comment #3)
> Created an attachment (id=147213) [details] [review]
> Session Manager extension, version 3
> 
> Version 3:
> Ask the user with a simple Yes/No dialog with "Yes" as default:
> Close all currently open windows?

Please note that Yes/No type alerts are highly discouraged; please use button labels that describe what the button does. For more information please see [1]. Another reason to eschew dialogs is that the majority of users don't read them, and usually just click them away. Only after the fact one realizes that data could be lost.

As you may have expected, my personal preference is version 4.

[1] http://library.gnome.org/devel/hig-book/stable/windows-alert.html.en#alert-button-order

Ok, thanks again for your feedback and the explanation.
After all, I could agree with you on version 4.

I will publish a patch for version 4 as soon I've found some time for that.

Would this extension also allow "quiet" saving of a session when closing the browser, and opening that session on restart? This is a feature that I miss greatly using firefox.

Hi Michael,

with the status quo API, this would be really difficult to implement (just not to say impossible).
But maybe that's only my perception and somebody knows some trick...

Created attachment 148665 [details] [review]
Session Manager extension, version 4 rc1

Nineteen days ago, I promised to publish a patch for version 4 as soon I'd find some time. Here it is.

Dear epiphany-extensions maintainers,


two months ago, I've proposed the attached patch (Session Manager extension, version 4) which I've implemented exactly in accordance with Reinout's proposition.

I've been using the extension since then, and would not like to miss it. For the future, I'm definitely willing to maintain this extension; just in case that there are any issues with it, or changes in the extensions API.

_What about taking it to the MASTER branch?_

For the case that there are any merge conflicts (however that should be limited to configure.ac) you don't want to resolve when applying the patch, I will send you an up to date version of it. Just tell me.


I don't wanna be too impatient, but I'm missing some feedback...

Review of attachment 148665 [details] [review]:

Thanks for this Klaus, sorry for the long time to reply.

Check the comments for your first function about casts, you have the same issue in all the other functions. Also watch alignment and code style, if in doubt read some Epiphany code to see the convention.
Update it when you get some time please :-)

::: extensions/session-manager/ephy-session-manager-extension.c
@@ +75,3 @@
+
+	const gchar *msg = g_strconcat (N_("The file does already exist"), ":\n'",
+		filename, "'.\n", N_("Do you want to overwrite it?"), NULL);

Why not g_strdup_printf? I think it would look better to just have your format string as one than the current concat.
Also, IMHO "The file does" sounds a bit weird, just make it "The file already exists".

Also, strconcat gives you a char*, and you can just give it like it to gtk_message_dialog_new. Btw, gtk_message_dialog_new take printf like arguments so you can build the string there directly.

@@ +79,3 @@
+	dialog = gtk_message_dialog_new (GTK_WINDOW (parent),
+                                  GTK_DIALOG_MODAL
+				  | GTK_DIALOG_DESTROY_WITH_PARENT,

Make it the same line, also please align the arguments with the first one (perhaps it's just my browser reading whitespace wrong, but you get the idea)

@@ +86,3 @@
+
+	gtk_widget_destroy (dialog);
+	g_free((gpointer) msg);

Woops. This is evil, g_strconcat gives you a char*.

@@ +104,3 @@
+				      ? GTK_STOCK_SAVE : GTK_STOCK_OPEN,
+				      GTK_RESPONSE_OK,
+				      NULL);

Alignment in case it fits, otherwise ok.

@@ +106,3 @@
+				      NULL);
+	gtk_dialog_set_default_response (GTK_DIALOG (dialog), GTK_RESPONSE_OK);
+	gtk_file_chooser_set_current_folder (GTK_FILE_CHOOSER (dialog), "~");

Use g_get_home_dir()

@@ +111,3 @@
+	{
+		filename = gtk_file_chooser_get_filename (
+			GTK_FILE_CHOOSER (dialog));

It's ok if you make one long line for this kind of cases.

@@ +123,3 @@
+				if (gtk_dialog_run (GTK_DIALOG (dialog))
+					!= GTK_RESPONSE_OK)
+					break;

One liners please.

@@ +145,3 @@
+	const gchar *previous_filename = g_build_filename (ephy_dot_dir (),
+		"extensions", "data", "session-manager",
+		PREVIOUS_SESSION_FILENAME, NULL);

Same as the comments for your first function.

@@ +332,3 @@
+	  NULL,
+	  G_CALLBACK (load_previous_session_cb) }
+};

This is usually put in the first lines of the file.

Created attachment 152934 [details] [review]
Session Manager extension, version 4 rc2

Hi Diego,

many thanks for your feedback, which I appreciate very much!

I've revised the patch with respect to your points of critique.
Please have a look, and tell me if there are still any shortcomings...

Thank you again,
Klaus

Review of attachment 152934 [details] [review]:

Looks nice Klaus, thanks. I think we can aim for this to be in epiphany-extensions 2.30. There are just a small comments here, but after those I think we can land a first version and get it tested in a 2.29.9something.

::: extensions/session-manager/ephy-session-manager-extension.c
@@ +125,3 @@
+					      GTK_STOCK_CANCEL,
+					      GTK_RESPONSE_CANCEL,
+					      GTK_FILE_CHOOSER_ACTION_SAVE ? GTK_STOCK_SAVE : GTK_STOCK_OPEN,

Don't you mean:
 (action == GTK_FILE_CHOOSER_ACTION_SAVE) ? GTK_STOCK_SAVE : GTK_STOCK_OPEN

?

@@ +197,3 @@
+	gchar *prev_filename;
+	gchar *filename = show_file_chooser_dialog(N_("Save Session"),
+						   GTK_FILE_CHOOSER_ACTION_SAVE);

A really picky comment: you miss the space between dialog and (:
dialog( -> dialog (

Same before I think.

@@ +274,3 @@
+	gboolean success;
+	EphyWindow *window;
+	GList *windows;

Make this GList *windows = NULL; and change that g_return_if_fail below.

@@ +290,3 @@
+
+	windows = ephy_session_get_windows (session);
+	g_return_if_fail (windows->data != NULL);

I think it's safer to check for windows != NULL, set GList *windows to NULL on declaration.

@@ +315,3 @@
+			gtk_widget_destroy (windows->data);
+			windows = windows->next;
+		} while (windows != NULL);

The goal here is to remove anything that is not from your loaded session file, right?

Created attachment 154316 [details] [review]
Session Manager extension, version 4

Thanks again for your helpful feedback!

> @@ +125,3 @@
> +                          GTK_STOCK_CANCEL,
> +                          GTK_RESPONSE_CANCEL,
> +                          GTK_FILE_CHOOSER_ACTION_SAVE ? GTK_STOCK_SAVE :
> GTK_STOCK_OPEN,
> 
> Don't you mean:
>  (action == GTK_FILE_CHOOSER_ACTION_SAVE) ? GTK_STOCK_SAVE : GTK_STOCK_OPEN
> 
> ?

Yeah, how did you guess that? ;)

> @@ +197,3 @@
> +    gchar *prev_filename;
> +    gchar *filename = show_file_chooser_dialog(N_("Save Session"),
> +                           GTK_FILE_CHOOSER_ACTION_SAVE);
> 
> A really picky comment: you miss the space between dialog and (:
> dialog( -> dialog (
> 
> Same before I think.

Fixed. There were two occurrences of this "irregularity".

> @@ +274,3 @@
> +    gboolean success;
> +    EphyWindow *window;
> +    GList *windows;
> 
> Make this GList *windows = NULL; and change that g_return_if_fail below.
>
> @@ +290,3 @@
> +
> +    windows = ephy_session_get_windows (session);
> +    g_return_if_fail (windows->data != NULL);
> 
> I think it's safer to check for windows != NULL, set GList *windows to NULL on
> declaration.

Thanks! I should know it better. There were some more places with this type of issue (also with WindowData *data).

> @@ +315,3 @@
> +            gtk_widget_destroy (windows->data);
> +            windows = windows->next;
> +        } while (windows != NULL);
>
> The goal here is to remove anything that is not from your loaded session file,
> right?

Yes, exactly.
For the case that a user is surprised about this behavior, there's the "Load Previous Session" feature, so that she can restore the previous window state.

Diego Escalante Urrelo wrote:
> Looks nice Klaus, thanks. I think we can aim for this to be in
> epiphany-extensions 2.30. There are just a small comments here, but after those
> I think we can land a first version and get it tested in a 2.29.9something.

As I've applied all your suggested changes (already six weeks ago), I would like to ask you to consider applying the attached patch to the current master branch.

Please tell me if you still see any reasons against that!

Klaus?

Yeah, that's my name.

Seems that I misread who was asking information from whom, sorry.

Diego, has this bug been resolved properly? NOTABUG seems rather strange for this one.

I changed the status to RESOLVED because
> A resolution has been taken, and it is awaiting verification by QA.


I chose NOTABUG for resolution because I found no better alternative.

NEEDINFO is no appropriate status either, because there seem to be no open questions on both sides.

The bug has not been fixed in the sense that
> A fix for this bug is checked into the tree and tested.
because it still has not been checked in. However I've been doing "tests" (i.e. using the plugin) for more than two months, always with the latest launchpad ppa on Ubuntu (the current one is epiphany-browser 2.30.2.

Actually, I don't see a status that is reflecting the current situation...

(In reply to comment #19)
> I changed the status to RESOLVED because
> > A resolution has been taken, and it is awaiting verification by QA.

Is this something from Ubuntu?

Please note that bugs in GNOME bugzilla should only be marked RESOLVED if they are fixed upstream (i.e. GNOME git). Whether or not downstream distributions have picked up a patch is irrelevant.

> Is this something from Ubuntu?
>
> Please note that bugs in GNOME bugzilla should only be marked RESOLVED if they
> are fixed upstream (i.e. GNOME git). Whether or not downstream distributions
> have picked up a patch is irrelevant.

Sorry, for not clearly expressing myself. I just wanted to say that I've been testing this extension with a current version of epiphany-browser. (I just obtain the binaries (of epiphany-browser) from a launchpad ppa, so I don't need to compile them myself anytime a new version has been released.)


I will finally change the status back to UNCONFIRMED.

I'm not sure but as far as I understood the last patch was suppose to save session. Nothing like that happened (I'm using 2.30.2 epiphany).

Hello Maciej,

I cannot reproduce your problem. I'm also using version 2.30.2 and for
me it works fine.


Just to clarify, this is how it is supposed to work:

# save session
- Tools -> Save Session
- choose filename
- click "Save" to save the current session

# load a saved session
- Tools -> Load Session
- choose a file containing a saved session
- click "Open" to load the saved session

# load previous session (i.e. go back to the session before a saved
session has been loaded, or rather go back to the most recently saved
session)
- Tools -> Load Previous Session


If something doesn't work for you, please explain it to me in more
detail.


Regards,
Klaus

(In reply to comment #23)
> Hello Maciej,
> 

Hello

> If something doesn't work for you, please explain it to me in more
> detail.
> 

I expected that a session is saved automatically at exit. 

> 
> Regards,
> Klaus

Regards

Yes, I've guessed that.

I'd really like to add a fourth functionality to "Save and Close Session" that also can be activated with the <Ctrl>+q hotkey (just like in firefox). The problem is that I don't know how to implement (in an extension) the loading of a saved session on application startup.

Ideas are welcome!

(In reply to comment #25)
> Yes, I've guessed that.
> 
> I'd really like to add a fourth functionality to "Save and Close Session" that
> also can be activated with the <Ctrl>+q hotkey (just like in firefox).

Could there be an option (even in gconf): "save session on exit by default"?

(Regarding implementation question - I presume you ask about how to implement it in extension. I know too less about Epiphany API to say anything but I'm a little afraid that it may require additional hooks there as currently it is UI-oriented).

> Could there be an option (even in gconf): "save session on exit by default"?

At this time, that does not make sense because of the problem I've described in my previous answer.
Saving a session on exit (which would be no problem to implement in an extension) is one thing.

The other thing is that users would expect that the saved session is restored when epiphany is started again. However, as I said before, I don't know how to implement that in an extension.

> (Regarding implementation question - I presume you ask about how to implement
> it in extension. I know too less about Epiphany API to say anything but I'm a
> little afraid that it may require additional hooks there as currently it is
> UI-oriented).

I'd like to propose a patch to epiphany, once the session manager extension is accepted. However, as long as this is not the case, I'm not willing to put any further effort into that issue.

I think it has previously been discussed somewhere: Restoring a session on startup is no problem, since it is done already when epiphany crashes.

The following change would be necessary to implement the feature of loading the previous session on startup:
For the case that epiphany has not crashed, prevent showing the crash recovery dialog (which asks whether to restore the previous (crashed) session or not), and load the session from the file the previous session has been stored to on exit (instead of loading the session from 'session_crashed.xml'). I guess that the change to the existing epiphany code would be rather trivial.

(In reply to comment #27)
> > Could there be an option (even in gconf): "save session on exit by default"?
> 
> At this time, that does not make sense because of the problem I've described in
> my previous answer.
> Saving a session on exit (which would be no problem to implement in an
> extension) is one thing.
> 
> The other thing is that users would expect that the saved session is restored
> when epiphany is started again. However, as I said before, I don't know how to
> implement that in an extension.
> 

Please see it in context. You describe process which is going to be implemented - I added comment about that there should be option to save be default instead of <Ctrl>+q.

I think I do understand.

One problem is that, unlike Firefox, epiphany has no "Quit" option. The only way to close epiphany (without killing (which means crashing) it) is to close window after window. To save a session on exit by default would imply that on exit only the state of the last window is saved.

The other problem is that in plugin context, there seems to be no way to detect whether the current window that is closed is also the last window (i.e. if closing the window goes along with application exit), it would be necessary to save the session any time a window is closed.

However, I don't see that as an option, as it would break the semantics of the "Load Previous Session" feature. More precisely, it would mean that anytime some window has been closed and the user selects "Load Previous Session", basically, just the previously closed window would be reopened. I don't think that would make sense.

As an alternative, I'd better introduce a fourth "Save and Close Session" option (with or without <Ctrl>+q shortcut). Therewith, the previous session could simply be restored with "Load Previous Session". The advantage I see in this behavior is that the "Load Previous Session" functionality would be transparent, as users can explicitly select to save the session on application exit or not.

I know that it would still be no perfectly satisfying, but a viable solution within the present constraints.

What do you think about it? Do you have any better suggestions?

(In reply to comment #29)
> I think I do understand.
> 
> One problem is that, unlike Firefox, epiphany has no "Quit" option. The only
> way to close epiphany (without killing (which means crashing) it) is to close
> window after window. To save a session on exit by default would imply that on
> exit only the state of the last window is saved.
> 

Not neccesarry. Epiphany is closed (not crashing) on logout as well.

Currently my way of exiting epiphany even on logout is pkill epiphany which as might be expected is not perfect ;)

According to its developer, epiphany-extensions is not under active development
anymore. (For reference: https://mail.gnome.org/archives/gnome-i18n/2013-May/msg00035.html and bug 700924.)

It is unlikely that there will be any further active development.

Closing this report as WONTFIX as part of Bugzilla Housekeeping - Please feel
free to reopen this bug report in the future if anyone takes the responsibility
for active development again.