- 🇵🇹Portugal joao.ramos.costa
joao.ramos.costa → made their first commit to this issue’s fork.
- Merge request !433102570: Store static reference of replaced term ids. → (Open) created by Unnamed author
- 🇵🇹Portugal joao.ramos.costa
Currently, the filter setting option checks if the term has already been replaced in that filter instance (plus string transformations). What I added in the open MR was to check, in addition to this, whether it has already been replaced in the current request. I think we'd be fine with the latter, as I believe that's the rationale for choosing that setting. At the moment, I’m also checking it more as a POC, so let others test. let’s discuss!
In that sense a new issue title and summary would be more appropriate, as any text instance rendered on the ‘page’ can use glossify filters, nodes, paragraphs, blocks, taxonomy terms… etc.
- Status changed to Needs review
15 days ago 12:47pm 15 April 2025 - 🇩🇪Germany Anybody Porta Westfalica
Honestly I'm personally not a big fan of this additional complexity, but let's wait for further maintainer and community feedback.
- 🇩🇪Germany Grevil
I am pretty divided on this issue. On one hand I think it is solved pretty clever and definitely has its use case, on the other hand I am not quite sure if a filter used inside a format instance should know about content of other format instances (even if its of the same type and only during the same request).
If we approve of this, we should definitly have a dedicated setting for this.
I'd say we change the "first_only" setting from a checkbox (bool) to a select / checkboxes (string) option with three Options:- disabled
- first_match_only
- first_match_only_across_request
Or something along those lines. Naming is far from being optimal.
- 🇩🇪Germany Anybody Porta Westfalica
Yep, that's also my point. If we put this functionality into a submodule (favourite) or at least a setting to opt into, I'd be happier...
- 🇵🇹Portugal joao.ramos.costa
Ok, I agree with a setting.
I understand that a submodule would add a clearer separation, but would add more complexity with the current code we have in the base class. Taking care of in which content/entity instances it has been replaced, I'm not sure if it make sense too.
But if @grevil meant how it is checked at current MR, well I'm not seeing how we can deal with it other way around.From my perspective, it makes sense that someone who chooses not to repeat the glossing of a certain term is referring to the entire page, in order to prevent that term from being marked more than once—like a reference in a book. But that’s just my opinion, nothing more than that :)
I can work on current branch and let you guys review it.
cheers!