Attaching the patches here from my branch.

Created attachment 309368 [details] [review]
Add new class LOKView

Setting the basic infra for using LOKDocView

Created attachment 309369 [details] [review]
Add a LOKViewToolbar

LOKDocView has different interface, so lets have a separate toolbar for it.

Created attachment 309370 [details] [review]
Use function to detect openDocument format

Created attachment 309371 [details] [review]
Separate window mode for LOKView

Better to have its separate window mode.

Created attachment 309372 [details] [review]
Separate menu items for LOKView

LOKDocView provides different interface. Options needs to be adjusted according to the feature it provides. Lets have a separate menu for it.

Created attachment 309373 [details] [review]
Separate window mode for LOKView

Review of attachment 309368 [details] [review]:

Thanks for the patches, Pranav.

::: src/documents.js
@@ +59,3 @@
+                             'application/vnd.oasis.opendocument.database',
+                             'application/vnd.oasis.opendocument.image',
+                             'application/vnd.openofficeorg.extension'];

Would it be possible to query the list of supported MIME types from LibreOffice instead of having a hard-coded list? That way if support for any format is missing / removed / added it will be automatically reflected at runtime.

@@ +748,3 @@
+            callback (this, null, null);
+            return;
+        }

This won't let us open ODFs from Google Drive, would it? Although I don't know if we want that in the first place, because if we did, then we would be opening Drive-native files in a WebKitWebView.

Review of attachment 309370 [details] [review]:

Note that the commit messages should have the URL of this bug.

::: src/documents.js
@@ +674,3 @@
+        if (openDocumentFormats.indexOf(this.mimeType) != -1)
+            return true;
+        return false;

I think this should be merged with the first patch.

Review of attachment 309368 [details] [review]:

::: src/embed.js
@@ +121,3 @@
 
+        this._lokview = new LOKView.LOKView(this._stackOverlay);
+        this._stack.add_named(this._lokview, 'lokview');

I think I would be happier if the naming was like: preview-lok  / PreviewLOK, preview-ev / PreviewEv, etc..

Created attachment 311486 [details] [review]
Add new class, LOKView, for LOKDocView widget

Use this widget, now, to open 'open document' format documents.

Created attachment 311487 [details] [review]
Add LOKView Toolbar

Created attachment 311488 [details] [review]
Let LOKView has its own window mode

This also helps invoking a separate toolbar for LOKView.

Created attachment 311489 [details] [review]
Let LOKView has its own menu

Created attachment 311490 [details] [review]
Add zoom feature

Created attachment 311491 [details] [review]
Add navigation buttons

Review of attachment 311486 [details] [review]:

::: src/documents.js
@@ +45,3 @@
 const Utils = imports.utils;
 
+const openDocumentFormats = ['application/vnd.oasis.opendocument.text',

This should probably live in the lokview.js instead, so that the Evince or LOK previews can say whether or not they support particular mime-types.

::: src/lokview.js
@@ +43,3 @@
+const Documents = imports.documents;
+
+// FIXME: Find this path dynamically

What is this supposed to correspond to? This should probably be defined in config.js.in by configure.

@@ +88,3 @@
+    _onLoadFinished: function(manager, doc, docModel) {
+        if (docModel == null && doc != null) {
+            let location = doc.uri.replace ('file://', '');

Eek. Use the JavaScript version of g_filename_from_uri().

Review of attachment 311487 [details] [review]:

As this is code in a new functionality with no dependencies, merge it with the "Add LOKView" patch.

Review of attachment 311488 [details] [review]:

I don't think I would create a new mode for the LOKView.

This seems to only be useful to set the correct toolbar, and has the same features as the preview, as seen in the little changes in mainWindow.js.

Review of attachment 311489 [details] [review]:

Again, this is in a new file, and independent, so fine to merge into earlier patches. Looks fine otherwise.

Review of attachment 311490 [details] [review]:

As I mentioned that we shouldn't add a new LOKVIEW mode, instead, when the mode is WindowMode.WindowMode.PREVIEW, it should ask the view itself whether it supports it.

Review of attachment 311491 [details] [review]:

::: src/documents.js
@@ +677,3 @@
     },
 
+    isOpenDocumentPartDocument: function() {

Should be in lokview.js directly.

I also realised that, because the previews and loading are split, the SkyDrive, OwnCloud and GoogleDocument subclasses also need to have specific code handling OpenOffice documents.

Maybe we could have a "preload" method, which we'd call so that SkyDrive and GoogleDocuments "render" the documents into PDF, to be chained to the PDF loader. It would do nothing for local and OwnCloud documents.

Sorry for not updating the patches. Little busy with fixing few bugs on LibreOffice side.

We have an API now for filter types, getFilterTypes. But I am not sure if we should allow opening all the mimeTypes returned by LO with the new widget or only few selected ones. Here is the sample output from my machine's latest libreoffice master.

{
    "writer_globaldocument_StarOffice_XML_Writer_GlobalDocument": {
        "MediaType": "application\/vnd.sun.xml.writer.global"
    },
    "Office Open XML Spreadsheet Template": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.spreadsheetml.template"
    },
    "writer_web_StarOffice_XML_Writer_Web_Template": {
        "MediaType": "application\/vnd.sun.xml.writer.web"
    },
    "calc_MS_Excel_5095_VorlageTemplate": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "impress_MS_PowerPoint_97_AutoPlay": {
        "MediaType": "application\/vnd.ms-powerpoint"
    },
    "impress_MS_PowerPoint_97_Vorlage": {
        "MediaType": "application\/vnd.ms-powerpoint"
    },
    "calc_MS_Excel_97_VorlageTemplate": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "calc_MS_Excel_95_VorlageTemplate": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "calc_MS_Excel_40_VorlageTemplate": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "calc_StarOffice_XML_Calc_Template": {
        "MediaType": "application\/vnd.sun.xml.calc.template"
    },
    "MS PowerPoint 2007 XML AutoPlay": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.slideshow"
    },
    "impress_StarOffice_XML_Impress": {
        "MediaType": "application\/vnd.sun.xml.impress"
    },
    "impress_CGM_Computer_Graphics_Metafile": {
        "MediaType": "image\/cgm"
    },
    "writer_LotusWordPro_Document": {
        "MediaType": "application\/vnd.lotus-wordpro"
    },
    "svg_Scalable_Vector_Graphics": {
        "MediaType": "image\/svg+xml"
    },
    "writer_WordPerfect_Document": {
        "MediaType": "application\/vnd.wordperfect"
    },
    "eps_Encapsulated_PostScript": {
        "MediaType": "image\/x-eps"
    },
    "Office Open XML Spreadsheet": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
    },
    "writerweb8_writer_template": {
        "MediaType": "application\/vnd.oasis.opendocument.text-web"
    },
    "writer_OOXML_Text_Template": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.wordprocessingml.template"
    },
    "writer_StarOffice_XML_Writer_Template": {
        "MediaType": "application\/vnd.sun.xml.writer.template"
    },
    "MS Excel 2007 XML Template": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.spreadsheetml.template"
    },
    "writer_MS_Word_97_Vorlage": {
        "MediaType": "application\/msword"
    },
    "writer_MS_Word_95_Vorlage": {
        "MediaType": "application\/msword"
    },
    "draw_WordPerfect_Graphics": {
        "MediaType": "image\/x-wpg"
    },
    "writer_MS_Works_Document": {
        "MediaType": "application\/vnd.ms-works"
    },
    "Office Open XML Presentation Template": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.template"
    },
    "math_StarOffice_XML_Math": {
        "MediaType": "application\/vnd.sun.xml.math"
    },
    "draw_Publisher_Document": {
        "MediaType": "application\/x-mspublisher"
    },
    "writerglobal8_template": {
        "MediaType": "application\/vnd.oasis.opendocument.text-master-template"
    },
    "impress_StarOffice_XML_Impress_Template": {
        "MediaType": "application\/vnd.sun.xml.impress.template"
    },
    "writer_BroadBand_eBook": {
        "MediaType": "application\/x-sony-bbeb"
    },
    "draw_Freehand_Document": {
        "MediaType": "image\/x-freehand"
    },
    "draw_StarOffice_XML_Draw_Template": {
        "MediaType": "application\/vnd.sun.xml.draw.template"
    },
    "sgf_StarOffice_Writer_SGF": {
        "MediaType": "image\/x-sgf"
    },
    "writer_StarOffice_XML_Writer": {
        "MediaType": "application\/vnd.sun.xml.writer"
    },
    "ppm_Portable_Pixelmap": {
        "MediaType": "image\/x-portable-pixmap"
    },
    "MS Excel 2007 VBA XML": {
        "MediaType": "application\/vnd.ms-excel.sheet.macroEnabled.12"
    },
    "writer_eReader_eBook": {
        "MediaType": "application\/vnd.palm"
    },
    "png_Portable_Network_Graphic": {
        "MediaType": "image\/png"
    },
    "writer_T602_Document": {
        "MediaType": "application\/x-t602"
    },
    "writer_MS_WinWord_60": {
        "MediaType": "application\/msword"
    },
    "tga_Truevision_TARGA": {
        "MediaType": "image\/x-targa"
    },
    "pgm_Portable_Graymap": {
        "MediaType": "image\/x-portable-graymap"
    },
    "math_MathML_XML_Math": {
        "MediaType": "application\/mathml+xml"
    },
    "dxf_AutoCAD_Interchange": {
        "MediaType": "image\/vnd.dxf"
    },
    "draw8_template": {
        "MediaType": "application\/vnd.oasis.opendocument.graphics-template"
    },
    "calc_Mac_Works": {
        "MediaType": "application\/vnd.ms-works"
    },
    "draw_ClarisWorks": {
        "MediaType": "application\/clarisworks"
    },
    "impress_ClarisWorks": {
        "MediaType": "application\/clarisworks"
    },
    "Office Open XML Presentation AutoPlay": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.slideshow"
    },
    "wmf_MS_Windows_Metafile": {
        "MediaType": "image\/x-wmf"
    },
    "pcd_Photo_CD_Base16": {
        "MediaType": "image\/x-photo-cd"
    },
    "bmp_MS_Windows": {
        "MediaType": "image\/x-MS-bmp"
    },
    "writer_TealDoc": {
        "MediaType": "application\/vnd.palm"
    },
    "emf_MS_Windows_Metafile": {
        "MediaType": "image\/x-emf"
    },
    "MS PowerPoint 2007 XML Template": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.template"
    },
    "pct_Mac_Pict": {
        "MediaType": "image\/x-pict"
    },
    "writer_MacWrite": {
        "MediaType": "application\/macwriteii"
    },
    "writer_MS_Word_97": {
        "MediaType": "application\/msword"
    },
    "ras_Sun_Rasterfile": {
        "MediaType": "image\/x-cmu-raster"
    },
    "writer_ClarisWorks": {
        "MediaType": "application\/clarisworks"
    },
    "writer_OOXML": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.wordprocessingml.document"
    },
    "draw_CorelDraw_Document": {
        "MediaType": "application\/vnd.corel-draw"
    },
    "impress8": {
        "MediaType": "application\/vnd.oasis.opendocument.presentation"
    },
    "pbm_Portable_Bitmap": {
        "MediaType": "image\/x-portable-bitmap"
    },
    "XHTML_File": {
        "MediaType": "application\/xhtml+xml"
    },
    "graphic_HTML": {
        "MediaType": "text\/html"
    },
    "writer_zTXT": {
        "MediaType": "application\/vnd.palm"
    },
    "impress_MS_PowerPoint_97": {
        "MediaType": "application\/vnd.ms-powerpoint"
    },
    "StarBase": {
        "MediaType": "application\/vnd.sun.xml.base"
    },
    "met_OS2_Metafile": {
        "MediaType": "image\/x-met"
    },
    "writer_Plucker_eBook": {
        "MediaType": "application\/prs.plucker"
    },
    "writer8": {
        "MediaType": "application\/vnd.oasis.opendocument.text"
    },
    "chart_StarOffice_XML_Chart": {
        "MediaType": "application\/vnd.sun.xml.chart"
    },
    "writer_Mac_Works": {
        "MediaType": "application\/vnd.ms-works"
    },
    "calc_HTML": {
        "MediaType": "text\/html"
    },
    "pcd_Photo_CD_Base": {
        "MediaType": "image\/x-photo-cd"
    },
    "writer_FictionBook_2": {
        "MediaType": "application\/x-fictionbook+xml"
    },
    "impress_AppleKeynote": {
        "MediaType": "application\/x-iwork-keynote-sffkey"
    },
    "writer_PalmDoc": {
        "MediaType": "application\/x-aportisdoc"
    },
    "MS PowerPoint 2007 XML": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.presentation"
    },
    "writer8_template": {
        "MediaType": "application\/vnd.oasis.opendocument.text-template"
    },
    "pcx_Zsoft_Paintbrush": {
        "MediaType": "image\/x-pcx"
    },
    "Gnumeric XML": {
        "MediaType": "application\/x-gnumeric"
    },
    "math8": {
        "MediaType": "application\/vnd.oasis.opendocument.formula"
    },
    "writer_Rich_Text_Format": {
        "MediaType": "application\/rtf"
    },
    "mov_MOV": {
        "MediaType": "application\/movie"
    },
    "calc_SYLK": {
        "MediaType": "text\/spreadsheet"
    },
    "pdf_Portable_Document_Format": {
        "MediaType": "application\/pdf"
    },
    "chart8": {
        "MediaType": "application\/vnd.oasis.opendocument.chart"
    },
    "draw_Corel_Presentation_Exchange": {
        "MediaType": "image\/x-cmx"
    },
    "gif_Graphics_Interchange": {
        "MediaType": "image\/gif"
    },
    "xbm_X_Consortium": {
        "MediaType": "image\/x-xbitmap"
    },
    "writer_MS_Word_95": {
        "MediaType": "application\/msword"
    },
    "generic_HTML": {
        "MediaType": "text\/html"
    },
    "jpg_JPEG": {
        "MediaType": "image\/jpeg"
    },
    "calc8": {
        "MediaType": "application\/vnd.oasis.opendocument.spreadsheet"
    },
    "writer_MS_WinWord_5": {
        "MediaType": "application\/msword"
    },
    "calc_Lotus": {
        "MediaType": "application\/vnd.lotus-1-2-3"
    },
    "draw_PageMaker_Document": {
        "MediaType": "application\/x-pagemaker"
    },
    "calc8_template": {
        "MediaType": "application\/vnd.oasis.opendocument.spreadsheet-template"
    },
    "Office Open XML Presentation": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.presentationml.presentation"
    },
    "calc_MS_Excel_5095": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "writer_DocBook_File": {
        "MediaType": "application\/docbook+xml"
    },
    "writerglobal8": {
        "MediaType": "application\/vnd.oasis.opendocument.text-master"
    },
    "calc_ClarisWorks": {
        "MediaType": "application\/clarisworks"
    },
    "generic_Text": {
        "MediaType": "text\/plain"
    },
    "impress8_template": {
        "MediaType": "application\/vnd.oasis.opendocument.presentation-template"
    },
    "calc_AppleNumbers": {
        "MediaType": "application\/x-iwork-numbers-sffnumbers"
    },
    "calc_MS_Excel_40": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "calc_MS_Excel_95": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "draw_StarOffice_XML_Draw": {
        "MediaType": "application\/vnd.sun.xml.draw"
    },
    "calc_MS_Excel_97": {
        "MediaType": "application\/vnd.ms-excel"
    },
    "calc_ODS_FlatXML": {
        "MediaType": "application\/vnd.oasis.opendocument.spreadsheet-flat-xml"
    },
    "calc_StarOffice_XML_Calc": {
        "MediaType": "application\/vnd.sun.xml.calc"
    },
    "svm_StarView_Metafile": {
        "MediaType": "image\/x-svm"
    },
    "draw_ODG_FlatXML": {
        "MediaType": "application\/vnd.oasis.opendocument.graphics-flat-xml"
    },
    "xpm_XPM": {
        "MediaType": "image\/x-xpixmap"
    },
    "MS Excel 2007 XML": {
        "MediaType": "application\/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
    },
    "writer_Mac_Word": {
        "MediaType": "application\/msword"
    },
    "writer_ApplePages": {
        "MediaType": "application\/x-iwork-pages-sffpages"
    },
    "pcd_Photo_CD_Base4": {
        "MediaType": "image\/x-photo-cd"
    },
    "writer_MS_Word_2007_Template": {
        "MediaType": "application\/msword"
    },
    "writer_MS_Word_2007": {
        "MediaType": "application\/msword"
    },
    "tif_Tag_Image_File": {
        "MediaType": "image\/tiff"
    },
    "writer_AbiWord_Document": {
        "MediaType": "application\/x-abiword"
    },
    "calc_Claris_Resolve": {
        "MediaType": "application\/clarisworks"
    },
    "writer_MIZI_Hwp_97": {
        "MediaType": "application\/x-hwp"
    },
    "writer_ODT_FlatXML": {
        "MediaType": "application\/vnd.oasis.opendocument.text-flat-xml"
    },
    "writer_MacWritePro": {
        "MediaType": "application\/macwriteii"
    },
    "draw_Visio_Document": {
        "MediaType": "application\/vnd.visio"
    },
    "draw8": {
        "MediaType": "application\/vnd.oasis.opendocument.graphics"
    },
    "impress_ODP_FlatXML": {
        "MediaType": "application\/vnd.oasis.opendocument.presentation-flat-xml"
    },
    "psd_Adobe_Photoshop": {
        "MediaType": "image\/vnd.adobe.photoshop"
    }
}
 
I plan to collect all the MediaType properties from this returned JSON.

I squashed the patches together and rebased them to master here: https://git.gnome.org/browse/gnome-documents/log/?h=wip/lokdocview-rebase

Using the version of libreoffice there:
http://koji.fedoraproject.org/koji/buildinfo?buildID=703072

I'm getting:
$ GI_TYPELIB_PATH=$GI_TYPELIB_PATH:/usr/lib64/libreoffice/girepository-1.0/ LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib64/libreoffice/program/ gnome-documentsGtk-Message: Failed to load module "pk-gtk-module"
failed to open library '/opt/libreoffice/instdir/program/libmergedlo.so': /opt/libreoffice/instdir/program/libmergedlo.so: cannot open shared object file: No such file or directory
failed to find hook in library '(null)'
Segmentation fault (core dumped)

Looks like we're going to have to make some changes to the packaging, as well as figure out how we can detect where this library is installed.

Hi Bastien,

Thanks for trying this out.

(In reply to Bastien Nocera from comment #26)
> Using the version of libreoffice there:
> http://koji.fedoraproject.org/koji/buildinfo?buildID=703072
> 
> I'm getting:
> $ GI_TYPELIB_PATH=$GI_TYPELIB_PATH:/usr/lib64/libreoffice/girepository-1.0/
> LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib64/libreoffice/program/
> gnome-documentsGtk-Message: Failed to load module "pk-gtk-module"
> failed to open library '/opt/libreoffice/instdir/program/libmergedlo.so':
> /opt/libreoffice/instdir/program/libmergedlo.so: cannot open shared object
> file: No such file or directory
> failed to find hook in library '(null)'
> Segmentation fault (core dumped)
> 
> Looks like we're going to have to make some changes to the packaging, as
> well as figure out how we can detect where this library is installed.

Ah ! I hard-coded a path long time back in lokview.js. const LO_PATH. It should work if you change that appropriately according to your system. And yeah, we need to fix this about finding the library path during runtime.

(In reply to Pranav Kant from comment #24)
> We have an API now for filter types, getFilterTypes. But I am not sure if we
> should allow opening all the mimeTypes returned by LO with the new widget or
> only few selected ones.

I don't understand. Why would you want to restrict that?

(In reply to Bastien Nocera from comment #17)
> Review of attachment 311486 [details] [review] [review]:
> 
> ::: src/documents.js
> @@ +45,3 @@
>  const Utils = imports.utils;
>  
> +const openDocumentFormats = ['application/vnd.oasis.opendocument.text',
> 
> This should probably live in the lokview.js instead, so that the Evince or
> LOK previews can say whether or not they support particular mime-types.

I've moved this now. I think we'll have to keep it, as we want to be able to detect LO types even if LOK is unavailable.

> ::: src/lokview.js
> @@ +43,3 @@
> +const Documents = imports.documents;
> +
> +// FIXME: Find this path dynamically
> 
> What is this supposed to correspond to? This should probably be defined in
> config.js.in by configure.

I've filed https://bugs.documentfoundation.org/show_bug.cgi?id=96247 about this

> @@ +88,3 @@
> +    _onLoadFinished: function(manager, doc, docModel) {
> +        if (docModel == null && doc != null) {
> +            let location = doc.uri.replace ('file://', '');
> 
> Eek. Use the JavaScript version of g_filename_from_uri().

Fixed.

(In reply to Bastien Nocera from comment #22)
> Review of attachment 311491 [details] [review] [review]:
> 
> ::: src/documents.js
> @@ +677,3 @@
>      },
>  
> +    isOpenDocumentPartDocument: function() {
> 
> Should be in lokview.js directly.

Fixed.

I pushed a fair bunch of fixes to the branch. Using the command from comment 26 I can at least get to the point where LOK is loaded, and make the backend crash when loading a particular file.

(In reply to Bastien Nocera from comment #23)
> I also realised that, because the previews and loading are split, the
> SkyDrive, OwnCloud and GoogleDocument subclasses also need to have specific
> code handling OpenOffice documents.
> 
> Maybe we could have a "preload" method, which we'd call so that SkyDrive and
> GoogleDocuments "render" the documents into PDF, to be chained to the PDF
> loader. It would do nothing for local and OwnCloud documents.

From my testing, Google Docs and One Drive just give us PDFs, so don't go through unoconv. The only cases where we might get unoconv called is when we loaded non-PDFs through the local or OwnCloud backends.

This means that we should be able to remove the unoconv special-case in the backend PDF loader.

I've pushed a bunch more patches to the branch.

Blockers for merging:
- https://bugs.documentfoundation.org/show_bug.cgi?id=96247 (and the bug that goes with it in lokview.js)
- https://bugs.documentfoundation.org/show_bug.cgi?id=96243 (we really don't want to crash in those cases)
- Disabling editing doesn't work (it's always in edit mode, not sure why)
- figure out why the error page shows up when the LOK view is loaded
- make sure all the navigation and controls work as expected in the LOK view

Note that I'm testing with libreoffice-core-5.1.0.0-8.beta1.fc24.x86_64 so a couple of the LO bugs might be fixed in later versions.

To be done later:
- move more of the preview-type specific code into each preview backend
- rename the preview.js to preview-ev.js and lokview.js to preview-lok.js
- rename edit.js to edit-ev.js and figure out how we want handle editing per-backend (we'll eventually want editing in the LOK backend, but might want to share the webkit editing for more than just the Evince backend)

Oh, and we'll need a way to generate thumbnails on a per-backend basis too.

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

(In reply to Bastien Nocera from comment #30)
> From my testing, Google Docs and One Drive just give us PDFs, so don't go
> through unoconv. The only cases where we might get unoconv called is when we
> loaded non-PDFs through the local or OwnCloud backends.

As we discussed in person in Madrid, OneDrive can give us PDFs. It doesn't do any server-side conversion to PDFs. I wrote a toy program to verify that. I think we misread src/lib/gd-pdf-loader.c.

(In reply to Debarshi Ray from comment #34)
> (In reply to Bastien Nocera from comment #30)
> > From my testing, Google Docs and One Drive just give us PDFs, so don't go
> > through unoconv. The only cases where we might get unoconv called is when we
> > loaded non-PDFs through the local or OwnCloud backends.
> 
> As we discussed in person in Madrid, OneDrive can give us PDFs. It doesn't
> do any server-side conversion to PDFs. I wrote a toy program to verify that.
> I think we misread src/lib/gd-pdf-loader.c.

I meant "can give us ODFs".

Sorry for the confusion.

(In reply to Pranav Kant from comment #24)
> We have an API now for filter types, getFilterTypes. But I am not sure if we
> should allow opening all the mimeTypes returned by LO with the new widget

Why not? :)

> or
> only few selected ones.

The problem that I see is that the following list also includes PDFs, which are better handled by EvView and friends. How about having some logic like this:
  Check if EvView can handle this MIME type. If yes, open it. Else check LO's getFilterTypes.

One thing I am not sure is whether there is an API to query the list supported by EvView because it has all these configurable (?) backends. In the worst case, we can hard code this list because it is going to be much smaller than LO's.

Review of attachment 311486 [details] [review]:

::: src/embed.js
@@ +88,3 @@
 
+        this._previewEv = new Preview.PreviewView(this._stackOverlay);
+        this._stack.add_named(this._previewEv, 'preview-ev');

I would prefer to stay consistent with the nomenclature. ie preview for evince and lokview for libreoffice.

@@ +91,3 @@
+
+        this._previewLOK = new LOKView.LOKView(this._stackOverlay);
+        this._stack.add_named(this._previewLOK, 'preview-lok');

The new mode/view is missing from _getViewFromMode and _restoreLastPage.

(In reply to Debarshi Ray from comment #36)
> One thing I am not sure is whether there is an API to query the list
> supported by EvView because it has all these configurable (?) backends.

ev_backends_manager_get_all_types_info

There is an example in gd-pdf-loader.c

Review of attachment 311486 [details] [review]:

::: src/lokview.js
@@ +60,3 @@
+
+        this._progressBar = new Gtk.ProgressBar({ halign: Gtk.Align.FILL,
+                                                  valign: Gtk.Align.START });

Do we really need a progress bar? We already have a spinner which should kick in.

(In reply to Debarshi Ray from comment #39)
> Review of attachment 311486 [details] [review] [review]:
> 
> ::: src/lokview.js
> @@ +60,3 @@
> +
> +        this._progressBar = new Gtk.ProgressBar({ halign: Gtk.Align.FILL,
> +                                                  valign: Gtk.Align.START
> });
> 
> Do we really need a progress bar? We already have a spinner which should
> kick in.

I'm guessing it has one because "edit.js" had one. Not sure that's the best UI for this either. I'll remove it later.

(In reply to Bastien Nocera from comment #31)
> I've pushed a bunch more patches to the branch.
> 
> Blockers for merging:
> - https://bugs.documentfoundation.org/show_bug.cgi?id=96247 (and the bug
> that goes with it in lokview.js)
> - https://bugs.documentfoundation.org/show_bug.cgi?id=96243 (we really don't
> want to crash in those cases)

Both are fixed.

> - Disabling editing doesn't work (it's always in edit mode, not sure why)

Fixed.

> - figure out why the error page shows up when the LOK view is loaded

Fixed.

> - make sure all the navigation and controls work as expected in the LOK view

This isn't done yet. In addition, still in flight are:
- search API doesn't work for us yet
- remove progressBar
- use evince API to find when to use Evince preview

I have now rebased wip/lokdocview-rebase on top of master and squashed some patches together so that we can start merging this.

Created attachment 318220 [details] [review]
preview-menu: Assign an ID to the rotate section

Created attachment 318374 [details] [review]
Use LOKDocView for handling ODF files

Created attachment 318375 [details] [review]
Use the same hamburger menu definition for both EvView and LOKDocView

Created attachment 318376 [details] [review]
documents, lokview: Make the LOKView optional at run-time

I merged a few patches from wip/lokdocview-rebase. I didn't merge the rest because I wasn't sure about a few things.

eg., this commit - what is supposed to break?

commit a723b7b7cfe0b94d3f79c9ea4fd0c035233c4f9f
Author: Bastien Nocera <hadess@hadess.net>
Date:   Sun Dec 6 15:36:06 2015 +0100

    Start loading LO docs when load-started is emitted
    
    Note that this will break the Evince view because of the way signals are
    handled.

Some of the later commits seem to imply that we should now be able to load ODFs from ownCloud, but it didn't work for me. Although, I could open ownCloud documents in LO - both from Files and from Documents' open menu.

(In reply to Debarshi Ray from comment #46)
> I merged a few patches from wip/lokdocview-rebase. I didn't merge the rest
> because I wasn't sure about a few things.
> 
> eg., this commit - what is supposed to break?
> 
> commit a723b7b7cfe0b94d3f79c9ea4fd0c035233c4f9f
> Author: Bastien Nocera <hadess@hadess.net>
> Date:   Sun Dec 6 15:36:06 2015 +0100
> 
>     Start loading LO docs when load-started is emitted
>     
>     Note that this will break the Evince view because of the way signals are
>     handled.

It's fixed in 7c9dad8a1beeeb2ce21528316c36ec329e747eae ("Add ViewTypes for different backends"), which adds view types, and makes sure that the views, which *all* receive load-started, load-error, etc., ignore the signals that weren't meant for them. For example, here the Evince view ignores signals not meant for it:
    _onLoadStarted: function() {
+    _onLoadStarted: function(manager, doc) {
+        if (doc.viewType != Documents.ViewType.EV)
+            return;

> Some of the later commits seem to imply that we should now be able to load
> ODFs from ownCloud, but it didn't work for me. Although, I could open
> ownCloud documents in LO - both from Files and from Documents' open menu.

You mean 79c35a5e58ef860688fe50d2d3e4dc2ff9eadb58 ("Share the code between ownCloud and local backends")? Is it listed properly? Can you add debug to loadLocal? I'm guessing the mime-type is missing, or something like that.

Created attachment 318524 [details] [review]
embed: Handle WindowMode.WindowMode.PREVIEW_LOK in _getViewFromMode

Created attachment 318525 [details] [review]
embed: Add a FIXME

Created attachment 318526 [details] [review]
documents, lokview: Move list of LibreOffice mime-types to the backend

Created attachment 318527 [details] [review]
documents: Share the code between ownCloud and local backends

Created attachment 318528 [details] [review]
lokview: Use GVfs URIs to open documents

Created attachment 318529 [details] [review]
Start loading LO documents when load-started is emitted

Created attachment 318530 [details] [review]
lokview: Avoid backtrace when LOK isn't available

Created attachment 318531 [details] [review]
lokview: Stop the error view from appearing when loading LO docs

Created attachment 318532 [details] [review]
lokview: Add "Copy to the Clipboard" functionality

Created attachment 318533 [details] [review]
Clamp zoom levels the same way as the Evince view

Review of attachment 318533 [details] [review]:

One of those two libreoffice bugs is already fixed, and the other is waiting for pranavk to respond to dtardon. Let's wait a bit and see if we can get it sorted out before merging this.

Created attachment 318981 [details] [review]
lokview: Clamp zoom levels the same way as the Evince view

Created attachment 319212 [details] [review]
lokview: Clamp zoom levels the same way as the Evince view

Created attachment 319291 [details] [review]
lokview: Clamp zoom levels the same way as the Evince view

Created attachment 319295 [details] [review]
lokview: Hack to not crash at start-up

LOKDocView's reset_view crashes when no document is loaded. This
hack prevents the crash till this is fixed in the widget.

LibreOffice blocker bug:
https://bugs.documentfoundation.org/show_bug.cgi?id=97235

Created attachment 319296 [details] [review]
lokview: Refresh the view after changing part

Ideally set_part() should take care of refreshing the view, but it
doesn't. Call reset_view() manually after calling set_part().

LibreOffice bug for the same:
https://bugs.documentfoundation.org/show_bug.cgi?id=97236

Created attachment 319297 [details] [review]
lokview: Refresh the view after changing part

Ideally set_part() should take care of refreshing the view, but it
doesn't. Call reset_view() manually after calling set_part().

LibreOffice bug for the same:
https://bugs.documentfoundation.org/show_bug.cgi?id=97236

Created attachment 319298 [details] [review]
lokview: Open OOXML documents also with LOKDocView

Created attachment 319299 [details] [review]
lokview: Hack to not crash at start-up

LOKDocView's reset_view crashes when no document is loaded. This
hack prevents the crash till this is fixed in the widget.

LibreOffice blocker bug:
https://bugs.documentfoundation.org/show_bug.cgi?id=97235

Created attachment 319300 [details] [review]
lokview: Refresh the view after changing part

Ideally set_part() should take care of refreshing the view, but it
doesn't. Call reset_view() manually after calling set_part().

LibreOffice bug for the same:
https://bugs.documentfoundation.org/show_bug.cgi?id=97236

Created attachment 319301 [details] [review]
lokview: Open OOXML documents also with LOKDocView

Review of attachment 319299 [details] [review]:

Thanks for the quick fix, Pranav. Looks good to me.

Review of attachment 319291 [details] [review]:

Works as advertised with libreoffice-5.1.0.2. Thanks.

Review of attachment 319300 [details] [review]:

Thanks. This fixes paging through multi-slide presentations and multi-sheet spreadsheets.

Review of attachment 319301 [details] [review]:

Looks good to me.

Nitpick. I would change the commit message to something like:
"lokview: Also open OOXML documents"

Created attachment 319486 [details] [review]
lokview: Only connect to signals when LOKDocView is available

Otherwise, it would crash gnome-documents in the startup trying
to connect signals to undefined LOKDocView instance.

Review of attachment 319486 [details] [review]:

Looks reasonable to me. It doesn't negatively affect the with-LOKDocView case, as far as I can tell.

Let's close this bug now.

Since we are slowly approaching the end of the GNOME 3.20 cycle, let's use separate bugs for further issues. That will make it easier to track any remaining problems.