*** Bug 722836 has been marked as a duplicate of this bug. ***

Currently we continue showing the buttons on the right hand side from the overview while the document is loading. Apart from adding a back button on the left, we should also make them insensitive.

Created attachment 270901 [details] [review]
WIP : basic idea

this is wip, the patch only shows basic idea, will be improving upon it soon.

Created attachment 270967 [details] [review]
show a back button while loading

Review of attachment 270967 [details] [review]:

Thanks for working on this, Pranav.

::: src/mainToolbar.js
@@ +62,3 @@
+                children.forEach(function(child){
+                    child.set_sensitive(false);
+                });

A better way to do this would be to monitor them inside _populateForOverview in OverviewToolbar. Listen to 'load-started' and make the buttons insensitive. Make sure you disconnect from the 'load-started' in _clearStateData.

@@ +65,3 @@
+
+                let backButton = this.addBackButton();
+                backButton.show();

The back button should be created and monitored in _populateForOverview in OverviewToolbar.

@@ +71,3 @@
+                        backButton.set_sensitive(false);
+                        Application.documentManager.loaderCancellable.cancel();
+                    }));

This should go in OverviewToolbar because this is specific to it, and does not apply to the other toolbars.

Created attachment 271304 [details] [review]
show a back button while loading

Review of attachment 271304 [details] [review]:

I think you attached the wrong patch because this is same as the previous one.

Created attachment 271608 [details] [review]
show a back button while loading

sorry for the wrong patch above. this one is correct.

Review of attachment 271608 [details] [review]:

Good job, the patch is working. I have two minor details.

There is an error when cancelling the loading of Google and Skydrive documents. 
This is easily solved by adding passwd to GdPrivate.pdf_loader_load_uri_async().

760: GdPrivate.pdf_loader_load_uri_async(this.uri, passwd, cancellable, Lang.bind(this,
986: GdPrivate.pdf_loader_load_uri_async(this.uri, passwd, cancellable, Lang.bind(this,

I think that setting the windowMode would be better than emitting an error signal.

::: src/documents.js
@@ +1171,3 @@
     _onDocumentLoadError: function(doc, error) {
+        if (error.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) {
+            this.emit('load-error', doc, _("Cancelled"), this._humanizeError(error));

Emitting the error message will go through a lot of code and only show the error page for a short while. Going back is not really an error, so we don't need to show error page. 
Instead we can set the window mode to NONE which will make the next line automatically go back to OVERVIEW.

Application.modeController.setWindowMode(WindowMode.WindowMode.NONE);

P.S. Remember to import windowMode ;)

Created attachment 272545 [details] [review]
Hiding the view and search buttons while loading.

Making an append to the setSensitive function from above towards bug 722836.

Could the buttons be explicitly marked within the populateforOverview function as there aren't many.

Created attachment 272548 [details] [review]
Cleaned up previous code

Created attachment 272549 [details] [review]
Cleaned up previous code

Sorry, I had forgotten to make the patch obsolete.

Priyanka, I understand that you are a GSoC candidate. Did you speak to anyone before picking this bug?

I gave Pranav, a GSoC candidate for gnome-photos, to solve this bug because the two applications are closely related. Marta, another GSoC candidate, re-reviewed Pranav's patch, which is really cool, although I did not ask her to. I was not able to take a second look at Pranav's patch because he kept me busy with patches on other bugs.

Seizing bugs that are being worked on by other candidates without asking anyone is probably not the most polite thing to do.

Debarshi, yes, my mentor Cosimo initially suggested that I work on this and the corresponding bug for hiding the extra buttons (the bug duplicate).
When I got here, I realized that Pranav had done work towards the back button and I thought I would just pitch in with adding the other buttons as well. 
At Debarshi and Pranav, I apologize. I did not mean any disrespect.

(In reply to comment #2)
> Currently we continue showing the buttons on the right hand side from the
> overview while the document is loading. Apart from adding a back button on the
> left, we should also make them insensitive.

I wouldn't do that - you are essentially in the preview at this point - not the content overview. The controls in the view should relate to the current context.

Created attachment 278878 [details] [review]
show a back button while loading

Review of attachment 278878 [details] [review]:

Thanks for the patch, Pranav.

::: src/documents.js
@@ +766,3 @@
                 if (exception) {
                     // try loading from the most recent cache, if any
+                    GdPrivate.pdf_loader_load_uri_async(this.identifier, passwd, cancellable, Lang.bind(this,

This fixes the parameters being passed to pdf_loader_load_uri_async, right? It should be in a separate commint.

@@ +992,3 @@
                 if (exception) {
                     // try loading from the most recent cache, if any
+                    GdPrivate.pdf_loader_load_uri_async(this.identifier, passwd, cancellable, Lang.bind(this,

Ditto.

::: src/preview.js
@@ +798,1 @@
             function() {

if (this._cancellable)
    this._cancellable.cancel();

@@ +820,3 @@
     },
 
+    setBackButtonCancellable: function(cancellable) {

Nit pick: isn't "setCancellable" enough, as a name?

@@ +840,3 @@
+                this._searchAction.enabled = true;
+            }));
+    },

I think we can do it without a separate unset method, and simplify the signal connection / disconnection part. Passing 'null' to setCancellable can be considered as unset, and we can just do a 'this._cancellable = cancellable' here. (see above)

I went ahead and fixed the above issues.

Created attachment 287860 [details] [review]
documents: Pass the correct arguments to pdf_loader_load_uri_async

Created attachment 287861 [details] [review]
embed: Switch to the preview as soon as an item starts loading

Created attachment 287862 [details] [review]
Let the back button cancel ongoing loads

Created attachment 287863 [details] [review]
Don't show a glimpse of the older item when going back to the preview

Review of attachment 287860 [details] [review]:

Looks good to me

Review of attachment 287861 [details] [review]:

OK

Review of attachment 287862 [details] [review]:

I'm not a big fan of passing the cancellable around. Isn't it possible to keep it entirely within DocumentsManager and cancel the operation from there when setActiveItem(null) is called while a load is in progress?

Review of attachment 287863 [details] [review]:

Looks good

(In reply to comment #25)
> Review of attachment 287862 [details] [review]:
> 
> I'm not a big fan of passing the cancellable around. Isn't it possible to keep
> it entirely within DocumentsManager and cancel the operation from there when
> setActiveItem(null) is called while a load is in progress?

Agreed. Actually, we don't need this patch at all because things already work the way you describe. ie. if you call setActiveItem(null), then _clearActiveDocModel() already takes care of the cancellation. :)

Review of attachment 287862 [details] [review]:

This is not needed.

Created attachment 287958 [details] [review]
application, preview: Disable the gear menu while a document is loading

We need the fix for bug 738083

Review of attachment 287958 [details] [review]:

Looks good to me if the GTK patch is accepted