Download it now!
Bug 61950 - Change Presentation Minimizer and Report Builder from bundled extensions to plain code
Summary: Change Presentation Minimizer and Report Builder from bundled extensions to p...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Extensions (show other bugs)
Version:
(earliest affected)
4.1.0.0.alpha0+ Master
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:4.1.0 target:4.2.0
Keywords:
Depends on:
Blocks: Database-Reports-Builder
  Show dependency treegraph
 
Reported: 2013-03-07 09:20 UTC by Stephan Bergmann
Modified: 2019-06-06 07:49 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Bergmann 2013-03-07 09:20:20 UTC
...as has already been done for the other (non-dictionary) extensions that are bundled by default, namely

* Presenter Screen:
** <http://cgit.freedesktop.org/libreoffice/core/commit/?id=ea91c7d90d74e1ca039ba669b5d3e14fa359c0fa> "Turn presenter screen from bundled extension to plain code"
** <http://cgit.freedesktop.org/libreoffice/core/commit/?id=6ebe207712eaf2233a2d45658e1d9b91706e75fc> "Forgot to add gid_File_Lib_PresenterScreen to module_impress"

* PDF Import:
** <http://cgit.freedesktop.org/libreoffice/core/commit/?id=7bf64a5af4088c0f6d4dbf7f493e94fa63e2a4b2> "Turn PDF import from bundled extension to plain code"
** <http://cgit.freedesktop.org/libreoffice/core/commit/?id=79e5ee5f6949f4b8645ef32dba61705d02473c60> "Isolate PDF Import so it can be made optionally installable"

* Python Scripting Provider:
** <http://cgit.freedesktop.org/libreoffice/core/commit/?id=1f3496e204cd305264d27a362f34fdcb6fa5f693> "Turn Python Scripting Provider from bundled extension to plain code"

<http://lists.freedesktop.org/archives/libreoffice/2012-November/041631.html> "--disable-ext-pdfimport -> --disable-pdfimport" gives the rationale for doing this:  "That commit is part of the plan to turn those extensions that we bundle 
anyway into true, non-extension parts of the core code base.  The 
benefit is that we can get rid of some ugly warts in the code, like 
linking most of basegfx's objects into both the basegfx lib and the 
pdfimport lib from the PDF Import extension, and that LO start-up, esp. 
after version update, becomes less troublesome and potentially faster."

From my personal notes:  "Remaining bundled extensions (apart from dictionaries) are Presentation Minimizer and Report Builder.  Changing them would require bigger changes (as they have substantial UI that would need to be reworked), but has less benefit than the others (no hacks to clean up), so postponed that past 4.0."

For further details of what needs to be done, consult the commit messages of the commits listed above.
Comment 1 DavidO 2013-03-08 17:51:38 UTC Comment hidden (obsolete)
Comment 2 Stephan Bergmann 2013-03-08 18:03:34 UTC
(In reply to comment #1)
> i wonder if the rename of the package from com.sun.star.report package to
> org.libreoffice.ext.report is still needed:
> 
> https://gerrit.libreoffice.org/#/c/2578/
> 
> well at least not org.libreoffice.ext.report but may be
> org.libreoffice.report,
> 
> right?

Yes; using a Java package name that does not clash with Java package names corresponding to UNOIDL modules is orthogonal to this issue.
Comment 3 DavidO 2013-03-13 21:24:43 UTC Comment hidden (obsolete)
Comment 4 Stephan Bergmann 2013-03-14 09:55:26 UTC
(In reply to comment #3)
> Question: as i understand, we should move the current registry (data and
> schema) from "reportbuilder/registry" to "officecfg/registry/".

Yes, generally makes sense to move it to that central location, just like the configuration data for all the other non-extension parts of LO.

> While schema moving was OK, the problem with data is, that we have some
> clashes with Setup.xcu, DataAccess.xcu, etc.
> 
> I guess we should just merge the data? But how then would it be possible to
> disable that feature and still enable the another one in the same xcu-file?
> 
> Example:
> DataAccess.xcu exists for evoab2 and repoortbuilder.
> If they are merged to the same file how can one then say:
> --with-evoab2 and simultaneously say 
> --without-reportbuilder
> 
> Or can it just be renamed to say DataAccess2 or whatever?

The solution here is to use so-called modules:

* In the .xcu file, mark a node/prop/value as install:module="X" (with xmlns:install="http://openoffice.org/2004/installation").  See e.g. install:module="calc" in officecfg/registry/data/org/openoffice/Setup.xcu.

* In officecfg/Configuration_officecfg.mk, add the file's module as gb_Configuration_add_spool_modules, see e.g. org/openoffice/Setup-calc.xcu mentioned there.

* In postprocess/CustomTarget_registry.mk, add the module's .xcu file to the appropriate .xcd file, see e.g. $(postprocess_MOD)/org/openoffice/Setup-calc.xcu mentioned there.

What the appropriate .xcd file is depends on whether the given functionality shall be available as an individually installable package (like for PDF Import, which has its own pdfimport.xcd) or shall be included as part of a larger package (like for Presenter Screen, which is included in impress.xcd).
Comment 5 DavidO 2013-03-14 10:24:15 UTC
[11:14:32] <_david_> _rene_: sberg Sweetshark what does it mean for "internalizing" of reportbuilder then?
[11:14:55] <_rene_> _david_: just do it and keep it into its own gid in scp2?
[11:15:05] <sberg> _david_, that you mostly model it after the patches for internalization of PDF Import
[11:15:06] <_rene_> _david_: nothing lost then. see how this was handled with pdfimport.
[11:16:15] <_rene_> sberg: only hsqldb and java itself
[11:16:26] <sberg> _david_, see 79e5ee5f6949f4b8645ef32dba61705d02473c60 for how this was done for PDF Import (which I had first internalized as not its own package)
Comment 6 Lionel Elie Mamane 2013-03-14 13:55:56 UTC Comment hidden (obsolete)
Comment 7 DavidO 2013-03-14 19:22:07 UTC
Question:

Controller.xcu have overlaying names:

Reportbuilder:
  <node oor:name="Registered">
    <node oor:name="ToolBar">
      <node oor:name="c4" oor:op="replace">
      <node oor:name="c5" oor:op="replace">
      <node oor:name="c6" oor:op="replace">
      <node oor:name="c7" oor:op="replace">

officecfg:

    <node oor:name="StatusBar">
      <node oor:name="c1" oor:op="replace">
      <node oor:name="c2" oor:op="replace">
      <node oor:name="c3" oor:op="replace">
      <node oor:name="c4" oor:op="replace">
      <node oor:name="c5" oor:op="replace">
      <node oor:name="c6" oor:op="replace">
      <node oor:name="c7" oor:op="replace">

how we should handle that?
Comment 8 Commit Notification 2013-03-18 15:37:09 UTC
David Ostrovsky committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=90a326c7028d5af132c62edfaef77c53627e4c0e

fdo#61950 move report builder from bundled extensions to plain code



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 9 DavidO 2013-03-18 17:47:53 UTC
<_rene_> _david_: for the minimizer, please add it to the impress gid (gid_Module_Prg_Impress_Bin). (like gid_File_Lib_PresenterScreen)
<_rene_> _david_: that will make it automatically end up in impress ;)
Comment 10 Stephan Bergmann 2013-06-07 11:46:04 UTC
David, bug 65168 makes me wonder whether the Report Builder half of this issue was considered done and good for inclusion into LO 4.1 by you, or whether you known about any problems that still need to be solved?
Comment 11 DavidO 2013-06-07 12:29:12 UTC
(In reply to comment #10)
> David, bug 65168 makes me wonder whether the Report Builder half of this
> issue was considered done and good for inclusion into LO 4.1 by you, or
> whether you known about any problems that still need to be solved?

The only thing that i was aware of was/is that strange thing about merging of some registry files from reportbuilder special to common locations/place. You checked it lately and argued that this task wasn't trivial at all. I was completely unaware of that wizard integration problem.

What we are missing in this project are test use cases for each area. Like what Linone attached to the bug that you have mentioned.
Comment 12 DavidO 2013-09-23 20:14:59 UTC
I wonder if it is still wanted:

"Change Presentation Minimizer from bundled extensions to plain code"

?
Comment 13 DavidO 2013-09-24 05:45:25 UTC
Should we change "SunPresentationMinimizer" to something more LO specific?

Archive:  ./workdir/unxlngx6/Extension/presentation-minimizer.oxt
    testing: META-INF/                OK
    testing: META-INF/manifest.xml    OK
    testing: SunPresentationMinimizer.uno.so   OK
    testing: bitmaps/extension_32.png   OK
    testing: bitmaps/minimizepresi_80.png   OK
    testing: bitmaps/opt_16.png       OK
    testing: bitmaps/opt_26.png       OK
    testing: components.rdb           OK
    testing: description-en-US.txt    OK
    testing: description.xml          OK
    testing: registration/            OK
    testing: registration/LICENSE     OK
    testing: registry/data/org/openoffice/Office/Addons.xcu   OK
    testing: registry/data/org/openoffice/Office/ProtocolHandler.xcu   OK
    testing: registry/data/org/openoffice/Office/extension/SunPresentationMinimizer.xcu   OK
    testing: registry/schema/org/openoffice/Office/extension/SunPresentationMinimizer.xcs   OK
Comment 14 Stephan Bergmann 2013-09-25 18:03:24 UTC
(In reply to comment #12)
> I wonder if it is still wanted:

Yes


(In reply to comment #13)
> Should we change "SunPresentationMinimizer" to something more LO specific?
[...]
> registry/data/org/openoffice/Office/extension/SunPresentationMinimizer.xcu  
> OK
>     testing:
> registry/schema/org/openoffice/Office/extension/SunPresentationMinimizer.xcs
> OK

Of sdext/source/minimizer/registry/schema/org/openoffice/Office/extension/SunPresentationMinimizer.xcs, BitmapPath prop and Strings group are just workarounds necessary because the original code is an extension; this should be done properly (without the need for .xcs/.xcu at all) when de-extension'ing the code.  Only the LastUsedSettings and Settings groups should survive, and should arguably be moved off the /org.openoffice.Office/extension path and renamed suitably, last but not least to avoid any potential conflict between the non-extension and any instance of the original extension that might be going to be installed (per user, say, whether or not that's useful), even if that means that old configuration data is effectively lost across the rename (I think we did the same with the other extensions, too).
Comment 15 DavidO 2013-10-04 05:45:27 UTC
It turns out that presentation minimizer extension is broken on current master.
That seems to fix it:

 https://gerrit.libreoffice.org/6122
Comment 16 Michael Stahl (CIB) 2013-10-10 21:39:43 UTC
some vague ideas about the menu: so my guess based on 15 minutes of
grepping around is that currently it works by Addons.xcu defining
a menu item with URL "vnd.com.sun.star.comp.SunPresentationMinimizer:execute"
and then ProtocolHandler.xcu defines that the corresponding
service is "com.sun.star.comp.SunPresentationMinimizerImp";
then it works by some fancy framework thing matching the
protocol, finding the service name, instantiating the service
in the usual way, and calling XDispatch::dispatch with the menu item URL on it.

i wonder if this could be done in a more direct way,
by simply having the menu item code (that is needed anyway apparently)
instantiate the service directly and calling its dispatch method
(not using the framework dispatch at all).

that would allow further simplification by replacing the ProtocolHandler
stuff with some XPresentationMinimizer interface.

apparently the service requires one parameter for the XFrame of the document,
that can be passed to createInstanceWithContextAndArguments().

... but i have no idea how this framework/menu stuff works.
Comment 17 DavidO 2013-10-13 10:51:30 UTC
(In reply to comment #16)
> some vague ideas about the menu: so my guess based on 15 minutes of
> grepping around is that currently it works by Addons.xcu defining
> a menu item with URL "vnd.com.sun.star.comp.SunPresentationMinimizer:execute"
> and then ProtocolHandler.xcu defines that the corresponding
> service is "com.sun.star.comp.SunPresentationMinimizerImp";
> then it works by some fancy framework thing matching the
> protocol, finding the service name, instantiating the service
> in the usual way, and calling XDispatch::dispatch with the menu item URL on
> it.
> 
> i wonder if this could be done in a more direct way,
> by simply having the menu item code (that is needed anyway apparently)
> instantiate the service directly and calling its dispatch method
> (not using the framework dispatch at all).
> 
> that would allow further simplification by replacing the ProtocolHandler
> stuff with some XPresentationMinimizer interface.
> 
> apparently the service requires one parameter for the XFrame of the document,
> that can be passed to createInstanceWithContextAndArguments().
> 
> ... but i have no idea how this framework/menu stuff works.

It was easy. The most important part was to deconstruct the URL with parseStrict(aURL):

        case SID_PRESENTATION_MINIMIZER:
        {
            Reference<XComponentContext> xContext(::comphelper::getProcessComponentContext());
            Reference<util::XURLTransformer> xParser(util::URLTransformer::create(xContext));
            Reference<frame::XDispatchProvider> xProvider(GetViewShellBase().GetController()->getFrame(), UNO_QUERY);
            if (xProvider.is())
            {
                util::URL aURL;
                aURL.Complete = "vnd.com.sun.star.comp.PresentationMinimizer:execute";
                xParser->parseStrict(aURL);
                uno::Reference<frame::XDispatch> xDispatch(xProvider->queryDispatch(aURL, OUString(), 0));
                if (xDispatch.is())
                {
                    xDispatch->dispatch(aURL, uno::Sequence< beans::PropertyValue >());
                }
            }
            Cancel();
            rReq.Ignore();
        }
        break;
Comment 18 DavidO 2013-10-13 10:54:17 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > I wonder if it is still wanted:
> 
> Yes
> 
> 
> (In reply to comment #13)
> > Should we change "SunPresentationMinimizer" to something more LO specific?
> [...]
> > registry/data/org/openoffice/Office/extension/SunPresentationMinimizer.xcu  
> > OK
> >     testing:
> > registry/schema/org/openoffice/Office/extension/SunPresentationMinimizer.xcs
> > OK
> 
> Of
> sdext/source/minimizer/registry/schema/org/openoffice/Office/extension/
> SunPresentationMinimizer.xcs, BitmapPath prop and Strings group are just
> workarounds necessary because the original code is an extension; this should
> be done properly (without the need for .xcs/.xcu at all) when
> de-extension'ing the code.

I don't know how to replace BitmapPath, and i don't understand why to change Strings section?
Comment 19 Stephan Bergmann 2013-10-14 07:09:31 UTC
(In reply to comment #18)
> I don't know how to replace BitmapPath, and i don't understand why to change
> Strings section?

This is poor man's (i.e., extension author's) way of defining UI in general and localizable strings in particular.  Core LO has standard mechanisms for this stuff, and there's no need to do it in such a complicated way in non-extension code.

* The extension currently includes four .png files (cf. sdext/Extension_minimizer.mk):

** icon-themes/galaxy/desktop/res/extension_32.png (the generic extension icon) is linked from sdext/source/minimizer/description.xml and thus no longer needed here after de-extension-alization.

** icon-themes/galaxy/minimizer/minimizepresi_80.png (large minimizer icon) is used in OptimizerDialog::InitRoadmap (sdext/source/minimizer/optimizerdialog.cxx).  The BitmapPath prop is used to locate it (and that's the only use of the BitmapPath prop), and it is apparently used as an icon in a dialog.  No idea how non-extension dialogs handle that best.  (Maybe via private:resource URLs?  I don't know.  Also, note that InformationDialog::InitDialog, sdext/source/minimizer/informationdialog.cxx, appears to be the only user of private:standardimage URLs as implemented in svtools/source/graphic/provider.cxx and svtools/source/misc/imageresourceaccess.cxx; maybe this was implemented solely for the minimizer extension use case; it should be done in a straightforward way now and the feature potentially removed from svtools.)

** icon-themes/galaxy/minimizer/opt_{16,26}.png (small minimizer icons) are referenced from sdext/source/minimizer/registry/data/org/openoffice/Office/Addons.xcu, so likely need to be handled some other way when the minimizer is no longer bolted on via the extension mechanisms (though I don't know any details).
Comment 20 DavidO 2013-10-14 07:20:33 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > I don't know how to replace BitmapPath, and i don't understand why to change
> > Strings section?
> 
> This is poor man's (i.e., extension author's) way of defining UI in general
> and localizable strings in particular.  Core LO has standard mechanisms for
> this stuff, and there's no need to do it in such a complicated way in
> non-extension code.
>

Do you think we can push that patch as is, and take care of this in a follow up change? It is already a big change and I think it would be simpler to ask on dev ML if that is in tree, because non of us actually knows how to do what you want properly.
Comment 21 Stephan Bergmann 2013-10-14 07:42:32 UTC
(In reply to comment #20)
> Do you think we can push that patch as is, and take care of this in a follow
> up change? It is already a big change and I think it would be simpler to ask
> on dev ML if that is in tree, because non of us actually knows how to do
> what you want properly.

Probably we can.  I'll have a look at the current state of the patch shortly (today/tomorrow).
Comment 22 DavidO 2013-10-14 07:44:36 UTC
(In reply to comment #21)
> (In reply to comment #20)
> > Do you think we can push that patch as is, and take care of this in a follow
> > up change? It is already a big change and I think it would be simpler to ask
> > on dev ML if that is in tree, because non of us actually knows how to do
> > what you want properly.
> 
> Probably we can.  I'll have a look at the current state of the patch shortly
> (today/tomorrow).

Thanks. I did verified it and it works like a charm after the conversion ;-)
Comment 23 Commit Notification 2013-10-14 10:32:17 UTC
David Ostrovsky committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2aa5d13ed09c1cbad9f1da315442edfa6f9f3b60

fdo#61950 De-extensionize presentation minimizer



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 24 Commit Notification 2013-10-15 11:14:04 UTC
Stephan Bergmann committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=8552e5fa1c56bb3ed7c86ef3c4922635e4178ebc

fdo#61950: Add back icon for "Tools - Minimize Presentation..." menu item



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 25 Commit Notification 2013-10-15 15:57:35 UTC
David Ostrovsky committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=cc2a405915e82c4b332dd25457f76704dc536d7f

fdo#61950 De-extensionize presentation minimizer: post clean



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 26 Stephan Bergmann 2013-10-22 08:57:58 UTC
(So, for the record, what still needs addressing is the Strings group in officecfg/registry/schema/org/openoffice/Office/PresentationMinimizer.xcs.)
Comment 27 DavidO 2013-10-23 09:20:14 UTC
(In reply to comment #26)
> (So, for the record, what still needs addressing is the Strings group in
> officecfg/registry/schema/org/openoffice/Office/PresentationMinimizer.xcs.)

My understanding was, that we can not easily do that and we can not (yet) migrate to the new UI because wizards are still not supported.
Comment 28 Timur 2016-02-25 19:34:09 UTC Comment hidden (obsolete)
Comment 29 Stephan Bergmann 2016-02-26 08:51:35 UTC Comment hidden (obsolete)
Comment 30 Stephan Bergmann 2016-11-30 09:41:17 UTC
[restore original summary to reflect that this bug handled both Presentation Minimizer and Report Builder]
Comment 31 Xisco Faulí 2017-09-11 08:51:14 UTC Comment hidden (obsolete)
Comment 32 QA Administrators 2018-09-12 02:39:00 UTC Comment hidden (obsolete)
Comment 33 Stephan Bergmann 2018-09-12 07:12:15 UTC
(In reply to QA Administrators from comment #32)
> Test to see if the bug is still present with the latest version of
> LibreOffice from https://www.libreoffice.org/download/

Yes, the issue discussed in comment 26 is still relevant for current master.
Comment 34 Stephan Bergmann 2018-11-28 08:05:44 UTC
When addressing is the Strings group in officecfg/registry/schema/org/openoffice/Office/PresentationMinimizer.xcs as per comment 26, please note Adolfo Jayme Barrientos' comment at <https://gerrit.libreoffice.org/#/c/63881/1/officecfg/registry/data/org/openoffice/Office/PresentationMinimizer.xcu> about the STR_INTRODUCTION_T prop:  "It’s better to split this paragraph out in a separate string, to maintain its visual separation in the UI."
Comment 35 Alex Thurgood 2019-06-05 13:41:42 UTC
(In reply to Lionel Elie Mamane from comment #6)
> With respect to Report Builder, ensure it is possible *not* to install it.
> It removes UI access to creating new legacy reports, so some users prefer
> not to have it.

Which clearly seems to have been ignored, at least for macOS and, I suspect, Windows.

Trying to use unopkg to list/remove the reportbuilder fails in current production releases for macOS because it is no longer referenced as either a bundled or a shared/user extension, in fact, it is completely invisible.
Comment 36 Stephan Bergmann 2019-06-06 06:34:26 UTC
(In reply to Alex Thurgood from comment #35)
> (In reply to Lionel Elie Mamane from comment #6)
> > With respect to Report Builder, ensure it is possible *not* to install it.
> > It removes UI access to creating new legacy reports, so some users prefer
> > not to have it.
> 
> Which clearly seems to have been ignored, at least for macOS and, I suspect,
> Windows.

Yes, that is apparently the case.  I suggest we continue discussion of whether and how to fix that in bug 58911.