Skip to content

Conversation

@fschmenger
Copy link
Collaborator

This PR adds a optional property moduleType to the module configuration, to make it possible to instantiate the same module multiple times. This can be useful, i.e. when you have the same or similar application logic and just require different parametrization.

To see an example, change the configuration of sample-module in app.conf to

"sample-module-inst1": {
    "moduleType": "sample-module",
    "target": "toolbar",
    "win": "floating",
    "icon": "md:star",
    "closable": false
  },
  "sample-module-inst2": {
    "moduleType": "sample-module",
    "target": "toolbar",
    "win": "floating",
    "icon": "md:star_outline",
    "closable": false
  }

To make it work, the line
moduleName="sample-module"
(which was redundant and is now generically forwarded) must be removed from SampleModule.vue:

 <wgu-module-card
    v-bind="$attrs"
    class="sample-module"
    :icon="icon"
  >

If you are using language files you must provide the title of the modules individually based on the instance name, e.g.

"sample-module-inst1": {
   "title": "Sample Module 1"
},
"sample-module-inst2": {
  "title": "Sample Module 2"
}

@fschmenger
Copy link
Collaborator Author

Hi guys, this draft should be easy to review (hopefully).

Let me know whether you are happy with this, then I will add a brief documentation.
Should I put a working example with 2 instances into app-conf.json? I could possible add it to sidebar (so I dont have to complicate matters to disambiguate the window position) - or do we rather keep app-conf-sidebar.json and app-conf.json in sync? Any ideas welcome...

@fschmenger fschmenger marked this pull request as draft October 6, 2025 15:56
@sronveaux
Copy link
Collaborator

sronveaux commented Nov 28, 2025

Hi @fschmenger and sorry for the delay on reviewing this one. I wanted to have a time slot where I could fully think about this one and experiment a bit before replying...

At first, for sure this is quite a nice addition, easy to implement and potentially useful!
Your explanation directly gives the opportunity to test it flawlessly which is great!

Here are some thinkings/ideas/remarks that came to my mind while playing with this so we can discuss it a bit further:

  • In this PR, only modules which have a window assigned can be duplicated. Wouldn't the other component deserve the same functionality ? It is also a one liner to be changed in the AppHeader.vue file where type: moduleOpts.win ? 'wgu-toggle-btn' : key + '-btn', should be replaced by type: moduleOpts.win ? 'wgu-toggle-btn' : (moduleOpts.moduleType ?? key) + '-btn',. Thinking about stock Wegue components, this is not as useful but I easily could test with two Geocoder, one querying osm and the second one photon and it worked flawlessly...
  • Simple cosmetic which is not linked to this PR at all, if AppHeader.vue was amended as stated up here, it could be nice to change this block which has a typo:
<template v-for="(tbButton, index) in menuButtons" :key="index">
            <v-list-item class="py-0">
              <component
                  v-bind="tbButton"
                  :is="tbButton.type" :key="index"
               />
              </v-list-item>
          </template>

it should be:

<template v-for="(menuButton, index) in menuButtons" :key="index">
            <v-list-item class="py-0">
              <component
                  v-bind="menuButton"
                  :is="menuButton.type" :key="index"
               />
              </v-list-item>
          </template>
  • Currently, the way moduleName is defined inside components is not standardized. Some define it as a constant in their data object (like the InfoClickWin) and some define it as a constant directly inside the <wgu-module-card> property (like the LayerListWin). Wouldn't it be the opportunity to uniformize this?
  • Directly linked to the previous one, should we modify the stock Wegue components so they could all be instantiated multiple times? This would only mean removing the lines defining moduleName inside AttributeTableWin, HelpWin, InfoClickWin, LayerListWin, MapRecorderWin and MeasureWin (+ some unit tests). As you explained for the SampleModule, this is in all cases forwarded and so useless to define... except if we want to ensure that having multiple instances of a given component would purposely break the app.
  • Concerning the documentation, would it be useful to indicate that stock Wegue components were not designed to be multi-instantiated and could behave incorrectly when used in such configuration? I'm thinking about the MeasureTool for example where you can define two instances drawing with different colors for example. What to expect when you open both instances at the same time? (yes I know, it's totally useless!)
  • And about writing a working example, another option would be not to include it in the configuration files but add a section inside the workshop later when it will be rewritten before v3 is out... otherwise, I personally have no opinion whether app-conf-sidebar.json and app-conf.json should be kept in sync. Perhaps @chrismayer who has the whole historical view on the development could better guide us here?

Once again I wrote something way too long. The idea is just to discuss thoroughly.
To summarize with my personal opinion about all this, sure this should be implemented, I'd just add the functionality to non-win components by patching the AppHeader component and remove all the definitions of moduleName so people reading the source code for the first time would not be confused by different approaches and to enable multi-instantiation of all components. Concerning the documentation and example, I really have no strong opinion except that a full working example should be available somewhere, either in the documentation pages, either in a configuration file or either in the workshop.

Thanks again for this nice idea and addition!

@fschmenger fschmenger force-pushed the multiple_module_instances branch from 9aff4ba to c94e592 Compare December 23, 2025 16:14
@fschmenger
Copy link
Collaborator Author

Thanks for the review @sronveaux.

I rebased the branch on the current master. Everything mentioned above should be addressed now (fixing the typo should be part of a different PR).
When I return from the christmas break I can potentially add a unit test for multi-instantiation.

@fschmenger fschmenger marked this pull request as ready for review December 23, 2025 16:17
@fschmenger
Copy link
Collaborator Author

I finalized some missing parts in the documentation and unit tests. Happy if you can give it another review but take your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants