Created attachment 298349 [details] [review]
Patch to hide the contents tab when there is no TOC to show

I used evince's has_document_links() method to check if there is a TOC available.

Review of attachment 298349 [details] [review]:

Thanks for the patch. It is mostly correct, except for a corner case.

::: src/places.js
@@ +64,3 @@
         header.set_custom_title(switcher);
 
+        //Check if the document has table of contents (TOC)

This comment is not needed because it is quite obvious from reading the code. However, it would be nice to have a comment below.

@@ +66,3 @@
+        //Check if the document has table of contents (TOC)
+        let doc = this._model.get_document();
+        let docHasLinks = doc.has_document_links();

The has_document_links method is only available if a EvDocument implements the EvDocumentLinks interface. Documents representing a PS file don't, which leads to this backtrace:

(gnome-documents:709): Gjs-WARNING **: JS ERROR: TypeError: doc.has_document_links is not a function
PlacesDialog<._createWindow@/opt/share/gnome-documents/js/places.js:68
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
PlacesDialog<._init@/opt/share/gnome-documents/js/places.js:41
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
_Base.prototype._construct@resource:///org/gnome/gjs/modules/lang.js:110
Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/lang.js:204
PreviewView<._showPlaces@/opt/share/gnome-documents/js/preview.js:206
wrapper@resource:///org/gnome/gjs/modules/lang.js:169
start@/opt/share/gnome-documents/js/main.js:30
@<command line>:1

As far as I know, there is no direct equivalent of EV_IS_DOCUMENT_LINKS in gjs. Therefore, you need to enclose this method call in a try / catch block and handle TypeError. It would be nice to have a comment explaining this. You can find a similar (but not exact) example in commit 3c5418c

@@ +67,3 @@
+        let doc = this._model.get_document();
+        let docHasLinks = doc.has_document_links();
+        //If there is no TOC, then don't show the "Contents" tab

This is also obvious, so the comment is not necessary.

I noticed a bug in gd_places_links_document_changed_cb. It short circuits the function if EV_IS_DOCUMENT_LINKS is false. Due to this the contents tab for PostScript files does not have the "No table of contents" place holder text. We should remove the check because it is duplicates a more correct version of it that exists a few lines below.

Now that we are going to hide the contents tab for these cases, this is mostly theoretical, but it would still be good to fix this in a separate patch, in case we ever use this code later.

Created attachment 301086 [details] [review]
Patch to hide TOC tab when there is no TOC

Review of attachment 301086 [details] [review]:

Thanks, Kara. The code looks good now. A few minor issues below:

::: src/places.js
@@ +74,3 @@
+        } catch (e) {
+        }
+

Trailing whitespace alert.

@@ +77,3 @@
+        if (docHasLinks)
+        {
+            this._linksPage = new GdPrivate.PlacesLinks();

The opening brace should be on the previous line. See the rest of the file.

Created attachment 301134 [details] [review]
Patch to hide TOC tab when there is no TOC to be displayed

I have corrected the opening brace but couldn't spot the trailing whitespace.

Review of attachment 301134 [details] [review]:

Perfect.

We now need to sort out the lone button in the stack switcher - it should be turned into a label. We can do that either in gtk+ or in the application itself. Let's use a separate bug for that.

Created attachment 301187 [details] [review]
places-links: Add the placeholer text for PostScript files

Addresses the issue in comment 3