Bug 106644 - Review for an extension; issues with LibreOffice-minimal-version
Summary: Review for an extension; issues with LibreOffice-minimal-version
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Extensions (show other bugs)
Version:
(earliest affected)
5.3.0.3 release
Hardware: All All
: medium normal
Assignee: Heiko Tietze
URL:
Whiteboard:
Keywords: needsDevEval
Depends on:
Blocks:
 
Reported: 2017-03-19 15:32 UTC by Heiko Tietze
Modified: 2017-03-26 12:08 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
Extension to export custom palettes (26.67 KB, application/zip)
2017-03-19 15:32 UTC, Heiko Tietze
Details
Some proposed small tweeks to the extension (28.55 KB, application/vnd.openofficeorg.extension)
2017-03-21 19:24 UTC, Niklas Johansson
Details
Modified the iterated variant (28.74 KB, application/x-zip-compressed)
2017-03-22 14:19 UTC, Heiko Tietze
Details
Extension to export custom palettes (34.55 KB, application/vnd.openofficeorg.extension)
2017-03-24 13:02 UTC, Heiko Tietze
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Heiko Tietze 2017-03-19 15:32:54 UTC
Created attachment 132013 [details]
Extension to export custom palettes

The attached extension tackles bug 90631. The capability to export a color list was intentionally removed (reasoning in https://design.blog.documentfoundation.org/2016/12/30/new-color-palettes-in-libreoffice/) but some users request easy means. Those must not be hard-coded but are a perfect example for extensions.

The attached file contains working code. It adds a menu entry and allows to export the custom palette as an extension itself. My request is to review the code. The xml parser could maybe be realized similar to the UserProfileName() method but I failed trying. In general I wonder if there may issues occur.

The extension itself it should work only from 5.3 on. It usually works for the color extension with <lo:LibreOffice-minimal-version d:name="LibreOffice 5.3" value="5.3"/> but not at the exporter extension. Tried with or without lo:/l:/d: etc.
Comment 1 Heiko Tietze 2017-03-20 12:14:08 UTC
oFileDialog.SetDisplayDirectory(GetPath("home")) crashes LibO fresh on Windows. GetPath("home") returns file:///C:/User/tietzeh/Documents, which is existent.

Version: 5.3.1.2
Build-ID: e80a0e0fd1875e1696614d24c32df0f95f03deb2
CPU-Threads: 4; BS-Version: Windows 6.1; UI-Render: Standard; Layout-Engine: neu; 
Gebietsschema: de-DE (de_DE); Calc: group
Comment 2 Niklas Johansson 2017-03-21 09:05:27 UTC
I can confirm the crash, however, looking at the extension code I see that you try to set the display directory before you initialize the file dialog. I'd say that is the reason for the crash. It shouldn't crash but I don't believe you'll ever be able to set the display directory before you initialize the dialog.

If you update the Main macro and switch places like this:
  oFileDialog = CreateUnoService("com.sun.star.ui.dialogs.FilePicker")
  oFileDialog.Initialize ( Array(com.sun.star.ui.dialogs.TemplateDescription.FILESAVE_AUTOEXTENSION ) )
  oFileDialog.SetDisplayDirectory(GetPath("home"))

you will no longer crash, but then on Windows you'll run into:
Bug 43021 - Macros: FilePicker method setDisplayDirectory shows wrong folder with native dialog Win 7 (64-bit) [1]

You could use "com.sun.star.ui.dialogs.OfficeFilePicker" instead of "com.sun.star.ui.dialogs.FilePicker". That uses LibreOffice own file dialogs which allows you to use setDisplayDirectory but the look and feel of that dialog is quite alien on Windows.


[1]: https://bugs.documentfoundation.org/show_bug.cgi?id=43021
Comment 3 Heiko Tietze 2017-03-21 10:31:28 UTC
(In reply to Niklas Johansson from comment #2)
> I can confirm the crash, however, looking at the extension code I see that
> you try to set the display directory before you initialize the file dialog.
> I'd say that is the reason for the crash. It shouldn't crash but I don't
> believe you'll ever be able to set the display directory before you
> initialize the dialog.

Thanks a lot, no crash on W7 anymore. Any other comment to the rest of the code?
Comment 4 Niklas Johansson 2017-03-21 19:22:37 UTC
Forgot about the workaround in the bug I mentioned. If you apply it to your code you can use the OS filepicker. Do something like:
 
 oFileDialog = CreateUnoService("com.sun.star.ui.dialogs.FilePicker")
 oFileDialog.Initialize ( Array(com.sun.star.ui.dialogs.TemplateDescription.FILESAVE_AUTOEXTENSION ) )
 oRegKey = GetRegistryKeyContent("/org.openoffice.Office.Common/Path/Info", true)
 oRegKey.WorkPathChanged = true
 oRegKey.commitChanges
 oFileDialog.SetDisplayDirectory(GetPath("home"))

NOTE: you need to load the global basic library Tools before you use the GetRegistryKeyContent (I see that you use it further down in the main macro).


Personally I probably make the ExportPalette a function and call it at the beginning. Just to check that you actually have something to export. I haven't looked to closely here so forgive me if I'm wrong but 

The line in the 'pack section of the main macro should be changed from
if FileExists(oFileDialog.Files(0)) Then RmDir(oFileDialog.Files(0))
to
If FileExists(oFileDialog.Files(0)) Then Kill(oFileDialog.Files(0))

This because oxt-file is a file not a directory.

I'd love to see a dialog to fill some of the description details, at least the identifier and version so that you have the option to create and install multiple custom color extensions without replacing installed versions. 

I've done a mockup of such a dialog in the extension that I will append together with code changes described above and some small adjustments like consistently writing If instead of if and so on (most of the time that seemed to be the convention in the code). 

Please do feel free to use or throw away any of my suggested changes in any way you see fit.
Comment 5 Niklas Johansson 2017-03-21 19:24:27 UTC
Created attachment 132052 [details]
Some proposed small tweeks to the extension
Comment 6 Heiko Tietze 2017-03-21 20:14:03 UTC
(In reply to Niklas Johansson from comment #5)
> Some proposed small tweeks to the extension

Cool stuff! Although, I think it's bad usability to have two dialogs, so we better show this new dialog on start with a field containing the home directory with an option to change it. I doubt that users who are not able to manage palettes and simple extensions know what an identifier is (me inclusive). Can't we just use always the same? And finally I would offer the palette name in this dialog to allow a different file name. Will apply all these changes in the next days and add you as author.

Open question to me is how to limit this extension for libo >=5.3. The <lo:LibreOffice-minimal-version d:name="LibreOffice 5.3" value="5.3"/> works only for the color extensions.
Comment 7 Niklas Johansson 2017-03-22 07:25:27 UTC
(In reply to Heiko Tietze from comment #6)
> (In reply to Niklas Johansson from comment #5)
> > Some proposed small tweeks to the extension
> 
> Cool stuff! Although, I think it's bad usability to have two dialogs, so we
> better show this new dialog on start with a field containing the home
> directory with an option to change it. I doubt that users who are not able
> to manage palettes and simple extensions know what an identifier is (me
> inclusive). Can't we just use always the same? And finally I would offer the
> palette name in this dialog to allow a different file name. 

Well the description file and it's content is not mandatory to my knowledge. So an option might be to not specify any identifier or publisher information at all. This is basically what the export of Basic libraries as extensions does. If we do not give any identifier then the extension will be replaced if it has the same file name. I guess it all depends on how powerful vs. how easy to use you want the extension to be. Maybe easy to use isn't a bad thing in this case.
Note that you will need the description file to specify the dependency on LibreOffice 5.3 but that is the only part that needs to be set.

> 
> Open question to me is how to limit this extension for libo >=5.3. The
> <lo:LibreOffice-minimal-version d:name="LibreOffice 5.3" value="5.3"/> works
> only for the color extensions.

It should work, seems to work when I add the sections. maybe you missed adding the namespace in the beginning?
xmlns:lo="http://libreoffice.org/extensions/description/2011"

And then of course it needs to be inside the dependencies tags.
<dependencies>
    <lo:LibreOffice-minimal-version d:name="LibreOffice 5.3" value="5.3"/>
</dependencies>
Comment 8 Heiko Tietze 2017-03-22 14:19:15 UTC
Created attachment 132073 [details]
Modified the iterated variant

Of course the name space was missing, stupid me. When I install an extension without identifier it's added regardless whether or not another exists. That ends up in a mess and we better go with the same default name. Do you think we should allow multiple color palettes based on the exported custom palette?
Comment 9 Niklas Johansson 2017-03-22 22:52:44 UTC
(In reply to Heiko Tietze from comment #8)

> ... When I install an extension
> without identifier it's added regardless whether or not another exists. 
I'd say that is not quite true. If two extensions don't have any identifier then it will still be replaced if the extension has the same name as the original extension. 
If we do give the option in the dialog to specify an identifier then we probably want to base the proposed identifier on the name of the extension. At least I would expect to be able to install multiple palett extensions, for example one for my company and one for a small organisation where I help out.

> ... Do you think
> we should allow multiple color palettes based on the exported custom palette?
I'm not sure exactly what you have in mind here. Do you mean that we should allow the user to split the custom palett into multiple paletts? Or are you thinking more in the lines of simply let the extension package multiple installed soc-files into a single extension?
If any of these two I'm leaning towards better documentation of how such a extension is created manually.


Something completely different, I noticed that when I added spaces to my extension name then it was displayed in url format with %20 representing the space. To fix this I added a ConvertFromURL where sPaletteName is defined.

      sParts = Split(ConvertToURL(oFileDialog.Files(0)),"/")
      sPaletteName = ConvertFromURL(GetFileNameWithoutExtension(sParts(UBound(sParts))))
Comment 10 Heiko Tietze 2017-03-23 10:19:21 UTC
(In reply to Niklas Johansson from comment #9)
> ConvertFromURL(GetFileNameWithoutExtension(sParts(UBound(sParts))))

Did exactly the same, but for some reasons the new macro wasn't added. Hope the work is not completely lost, worked on a different machine yesterday.
Comment 11 Heiko Tietze 2017-03-24 13:01:02 UTC
Comment on attachment 132013 [details]
Extension to export custom palettes

obsolete
Comment 12 Heiko Tietze 2017-03-24 13:02:51 UTC
Created attachment 132127 [details]
Extension to export custom palettes

Final version, hopefully.
Comment 13 Heiko Tietze 2017-03-26 12:08:23 UTC
Closing as the extension is finished. Added an image so that the custom palettes are clearly identified in the extension manager. Publication process has been started.