#7585: refactor of the Print plugin as a plugins container #7596

mbarto · 2021-11-23T14:41:19Z

Description

This PR is a refactor of the Print plugin, to allow for custom UI plugins to be included from projects.
Most of the printing dialog widgets have been refactored as subplugins of the Print plugin.

Some ancillary work has been included to improve usability:

support for different implementations of the same plugin in PluginsUtils.getPluginItems
support of testing lazy plugins with pluginsTestUtils.getLazyPluginForTest
getByXPath function (in Print-test.jsx to be eventually moved into a utility file) to allow a more react-testing-library like approach for testing
support for custom parameters (set in the spec params object) in PrintUtils.getMapfishPrintSpecification

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

Issue

#7585

What is the current behavior?

#7585

What is the new behavior?

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

Yes, and I documented them in migration notes
No

Other useful information

…iner

offtherailz

It's a little scary to me to have too many plugins by default. We already had some doubts with timeline + playback.

Can I suggest to use plugin injection as replacement for defaults components when you have a specific target.
E.g. when a plugin as a target like the following in the containers:

{ target:  "TextInput"
|        "Layout"
|        "LegendOptions"
|        "Resolution"
|        "MapPreview"
|        "Option"

The (documented) convention of the plugin is that these target are replacements for default components.
I think this solution is more maintainable on the core.
What do you think?

mbarto · 2021-11-24T11:02:36Z

@offtherailz how would you add a custom plugin in a specific position? For example between title and description in the current printing dialog?
I like using the target as a replacement, it gives more flexibility. Do you think we should also handle removal of a "default" widget ?

offtherailz · 2021-11-25T14:33:59Z

@offtherailz how would you add a custom plugin in a specific position? For example between title and description in the current printing dialog?

In this case I should replace the whole panel (TextInput??). If some panels may accept more options, maybe we can define a target for the panel content.I Don't know if it is a requirement

I think you can be able to:

Replace default panels
Add some panels

Moreover I read on mailing list they was asking for adding a button to save on cloud the pdf. So future we may also allow to add some buttons here and there.

Do you think we should also handle removal of a "default" widget ?

Now I think they are decoupled so every panel is idependent and I think it makes sense, Maybe injecting an null component for the target may work?

mbarto · 2021-11-25T16:22:02Z

@offtherailz to summarize the changes you suggest should be:

leave the default (connected) components untouched in plugins/print/index.js
change Print.jsx to allow replacements (or removal) of those components via plugins with a documented target (removal happens if the plugin has a null component definition)
allow addition of custom plugins in predefined targets (e.g. left-panel, right-panel, etc.) as already done by this PR

Is it correct?

offtherailz · 2021-11-25T16:28:22Z

Yes @mbarto , correct. Hope it makes the plugin more flexible and maintainable.

…iner, restored standard components

…iner, implemented override and removal of standard plugins

…iner, fix in Option to correctly use properties with path

mbarto · 2021-11-29T15:23:28Z

@offtherailz I have updated the PR. The build is currently failing due to webpack/nodejs issues (I guess)

…iner, fix for the build

offtherailz · 2021-11-30T13:28:00Z

web/client/product/plugins.js

+        PrintTextInputPlugin: require('../plugins/print/TextInput').default,
+        PrintLayoutPlugin: require('../plugins/print/Layout').default,
+        PrintLegendOptionsPlugin: require('../plugins/print/LegendOptions').default,
+        PrintResolutionPlugin: require('../plugins/print/Resolution').default,
+        PrintMapPreviewPlugin: require('../plugins/print/MapPreview').default,
+        PrintOptionPlugin: require('../plugins/print/Option').default


My idea was not to include any plugin, but only allow to override by plugin injection, on demand. Having this big set of plugin to configure or customize a single plugin is a problem. Potentially makes the size of the plugins list explode.

I asked to have a plugin like :

{ "name": "PluginCustomization", "containers": { "Print": { target: "left-panel-accordion", // I suggested to have specific targets to replace specific components. position: "" Component: MyCustomization } } }

…iner, moved the Null plugin in plugins/print, removed print subplugins from product plugins.js, updated docs

offtherailz

It's ok, now I understood what you did.

Please look ad my comments, I think that applying the changes everything will seems easier to configure and maintain
You can avoid to use createPlugin for every single panel in print folder, using directly the component (that is optionally connected to the state).
For targets use names with dashes for everything, for constiency, as you did for left-panel e.g. printoptions --> print-options, defaultbackgroundignore -> default-background-ignore

web/client/plugins/Print.jsx

offtherailz · 2021-11-30T14:05:53Z

web/client/plugins/Print.jsx

+ * // Custom plugin added to the left-panel target
+ * {
+ *   "name": "PrintTextInput",
+ *   "id", "ExtraParam",
+ *   "override": {
+ *       "Print": {
+ *          "target": "left-panel",
+ *          "position": 1.5
+ *       }
+ *   },
+ *   "cfg": {
+ *       "property": "extra",
+ *       "label": "Extra stuff",
+ *       "placeholder": "Inserisci cose..."
+ *   }
+ * }
+ *
+ * @example
+ * // Custom plugin replacing an existing target
+ * {
+ *   "name": "PrintTextInput",
+ *   "id", "ExtraParam",
+ *   "override": {
+ *       "Print": {
+ *          "target": "name",
+ *          "position": 1
+ *       }
+ *   },
+ *   "cfg": {
+ *       "property": "extra",
+ *       "label": "Extra stuff",
+ *       "placeholder": "Inserisci cose..."
+ *   }
+ * }
+  *
+ * @example
+ * // Removal of an existing target
+ * {
+ *   "name": "Null",
+ *   "override": {
+ *       "Print": {
+ *          "target": "name",
+ *          "position": 1
+ *       }
+ *   },
+ *   "cfg": {
+ *   }
+ * }


Instead of having 3 examples and a lot of configurations and code, I suggest to use one single plugin, with no component, using the multi-target support.
Please try it.

Suggested change

* // Custom plugin added to the left-panel target

* {

* "name": "PrintTextInput",

* "id", "ExtraParam",

* "override": {

* "Print": {

* "target": "left-panel",

* "position": 1.5

* }

* },

* "cfg": {

* "property": "extra",

* "label": "Extra stuff",

* "placeholder": "Inserisci cose..."

* }

* }

*

* @example

* // Custom plugin replacing an existing target

* {

* "name": "PrintTextInput",

* "id", "ExtraParam",

* "override": {

* "Print": {

* "target": "name",

* "position": 1

* }

* },

* "cfg": {

* "property": "extra",

* "label": "Extra stuff",

* "placeholder": "Inserisci cose..."

* }

* }

*

* @example

* // Removal of an existing target

* {

* "name": "Null",

* "override": {

* "Print": {

* "target": "name",

* "position": 1

* }

* },

* "cfg": {

* }

* }

// PrintCustomizations.js

const MyCustomPanel = () => <div>Hello, I am a custom component</div>

const MyCustomLayout = connect(mapStateToProps, dispatchToProps)(CustomLayoutSelector);

export default createPlugin('PrintCustomizations',

containers: [

// this entry allows to add a new panel between text input and accordion panel

{

Print: [{ // this entry add a panel

target: "left-panel",

position: 1.5,

Component: MyCustomPanel

},

// this entry allows to

{

target: "layout",

Component: MyCustomLayout

}, {

target: "map-preview", // To remove one component, simply create a component that returns null.

Component: () => null

}]);

offtherailz · 2021-11-30T14:14:12Z

web/client/plugins/Print.jsx

+ * To remove a widget, you have to include a Null plugin with the desired target.
+ * You can use the position to sort existing and custom items.


You can customize Print plugin by creating one custom plugin (or more) that modifies the existing components with your ones. You can configure this plugin in localConfig.json as usual.

Co-authored-by: Lorenzo Natali <offtherailz@gmail.com>

…mproved docs

tdipisa · 2021-12-14T14:57:32Z

This PR has been merged without an approval @mbarto. @offtherailz please check it as soon as you can.

offtherailz · 2021-12-14T15:37:30Z

My review was on 2 points:

Please look ad my comments, I think that applying the changes everything will seems easier to configure and maintain
For targets use names with dashes for everything, for constiency, as you did for left-panel e.g. printoptions --> print-options, defaultbackgroundignore -> default-background-ignore <-- I did it with my comments

These are missing:

You can avoid to use createPlugin for every single panel in print folder, using directly the component (that is optionally connected to the state). <-- this seems not to be done, but it is a simplification.
This comment - related to the avoid of using createPlugin with an example and some documentation #7585: refactor of the Print plugin as a plugins container #7596 (comment)

These changes are not mandatory for the functionality itself, but simplify a lot the current solution, that is very tricky.
If you missed this point in error @mbarto can you open a separate issue for that?
If otherwise you are against these requests for any reason (time, technical issues or other) please feel free to reply, but don't ignore them.

mbarto · 2021-12-14T16:00:15Z

These are missing:

You can avoid to use createPlugin for every single panel in print folder, using directly the component (that is optionally connected to the state). <-- this seems not to be done, but it is a simplification.

If you look at the code you will see that each panel exports both a component and a plugin (as a default).
The component is used in index.js (this I suppose is what you were asking for), while the plugin can be used in project (and it will be used for the client I am doing this work for). Those plugins definition are not included in the product plugins.js (as you requested).

This comment - related to the avoid of using createPlugin with an example and some documentation #7585: refactor of the Print plugin as a plugins container #7596 (comment)

I didn't commit your suggestion directly, but if you look again at the example, if has been replaced by something very similar to yours, but verified and tested.

These changes are not mandatory for the functionality itself, but simplify a lot the current solution, that is very tricky. If you missed this point in error @mbarto can you open a separate issue for that? If otherwise you are against these requests for any reason (time, technical issues or other) please feel free to reply, but don't ignore them.

That said, feel free to remove my admin commit rights from the project if you prefer.

offtherailz · 2021-12-14T17:14:14Z

If you look at the code you will see that each panel exports both a component and a plugin (as a default).
The component is used in index.js (this I suppose is what you were asking for), while the plugin can be used in project (and it will be used for the client I am doing this work for). Those plugins definition are not included in the product plugins.js (as you requested).

So all that createPlugins can be removed. They are not used and they are not documented in print plugin, It is only confusing.

Or there is some reason to keep them?

I didn't commit your suggestion directly, but if you look again at the example, if has been replaced by something very similar to yours, but verified and tested.

Great, I was confused by the double export (I said it was only confusing).

That said, feel free to remove my admin commit rights from the project if you prefer.

Please don't misunderstand me, I mean you may ask to me if you was in a hurry or what else. We would find a way to clarify and maybe split this clean up in 2.
I'd say yes, for sure.
Merging without telling anything only confuses more, and for async work we need trust and clarity.

mbarto added this to the 2022.01.00 milestone Nov 23, 2021

mbarto requested review from offtherailz, MV88 and allyoucanmap November 23, 2021 14:41

mbarto self-assigned this Nov 23, 2021

mbarto force-pushed the printing_plugins_refactor branch from 5a5aa97 to 4dfd8db Compare November 23, 2021 16:03

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

8d579a5

…iner

mbarto force-pushed the printing_plugins_refactor branch from 4dfd8db to 8d579a5 Compare November 23, 2021 16:24

mbarto marked this pull request as ready for review November 23, 2021 16:40

offtherailz reviewed Nov 24, 2021

View reviewed changes

offtherailz added C231-GEOINFO_CO-2021-AIT enhancement labels Nov 24, 2021

mbarto added 3 commits November 29, 2021 12:48

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

5e55695

…iner, restored standard components

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

30e65a1

…iner, implemented override and removal of standard plugins

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

ce4d08d

…iner, fix in Option to correctly use properties with path

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

c795611

…iner, fix for the build

offtherailz reviewed Nov 30, 2021

View reviewed changes

mbarto added 2 commits November 30, 2021 15:03

geosolutions-it#7585: refactor of the Print plugin as a plugins conta…

29a0aa0

…iner, moved the Null plugin in plugins/print, removed print subplugins from product plugins.js, updated docs

Merge branch 'master' into printing_plugins_refactor

cd6bab5

offtherailz requested changes Nov 30, 2021

View reviewed changes

mbarto and others added 3 commits November 30, 2021 15:35

Update web/client/plugins/Print.jsx

7e7d3ed

Co-authored-by: Lorenzo Natali <offtherailz@gmail.com>

Update web/client/plugins/Print.jsx

ae63972

Co-authored-by: Lorenzo Natali <offtherailz@gmail.com>

geosolutions-it#7585: Print plugin refactor as a plugins container, i…

3bb6e59

…mproved docs

This was referenced Dec 7, 2021

MapStore printing dialog plugins refactor geosolutions-it/mapfish-print#39

Open

MapStore mapfish specification creator refactor geosolutions-it/mapfish-print#40

Open

mbarto merged commit 1a3a0b5 into geosolutions-it:master Dec 14, 2021

mbarto deleted the printing_plugins_refactor branch December 14, 2021 14:50

tdipisa added tmp and removed tmp labels Feb 11, 2022

tdipisa added the Print label Mar 7, 2022

allyoucanmap mentioned this pull request Aug 9, 2024

#4675 Remove unused code #10442

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#7585: refactor of the Print plugin as a plugins container #7596

#7585: refactor of the Print plugin as a plugins container #7596

mbarto commented Nov 23, 2021

offtherailz left a comment •

edited

Loading

mbarto commented Nov 24, 2021

offtherailz commented Nov 25, 2021 •

edited

Loading

mbarto commented Nov 25, 2021

offtherailz commented Nov 25, 2021

mbarto commented Nov 29, 2021

offtherailz Nov 30, 2021 •

edited

Loading

offtherailz left a comment •

edited

Loading

offtherailz Nov 30, 2021

offtherailz Nov 30, 2021

tdipisa commented Dec 14, 2021

offtherailz commented Dec 14, 2021

mbarto commented Dec 14, 2021

offtherailz commented Dec 14, 2021

		* To remove a widget, you have to include a Null plugin with the desired target.
		* You can use the position to sort existing and custom items.

#7585: refactor of the Print plugin as a plugins container #7596

#7585: refactor of the Print plugin as a plugins container #7596

Conversation

mbarto commented Nov 23, 2021

Description

Issue

Breaking change

Other useful information

offtherailz left a comment • edited Loading

Choose a reason for hiding this comment

mbarto commented Nov 24, 2021

offtherailz commented Nov 25, 2021 • edited Loading

mbarto commented Nov 25, 2021

offtherailz commented Nov 25, 2021

mbarto commented Nov 29, 2021

offtherailz Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

offtherailz left a comment • edited Loading

Choose a reason for hiding this comment

offtherailz Nov 30, 2021

Choose a reason for hiding this comment

offtherailz Nov 30, 2021

Choose a reason for hiding this comment

tdipisa commented Dec 14, 2021

offtherailz commented Dec 14, 2021

mbarto commented Dec 14, 2021

offtherailz commented Dec 14, 2021

offtherailz left a comment •

edited

Loading

offtherailz commented Nov 25, 2021 •

edited

Loading

offtherailz Nov 30, 2021 •

edited

Loading

offtherailz left a comment •

edited

Loading