Skip to content

wxGUI/gmodeler: Correct variable replacement in model_convert.py#7241

Open
gureckyfrantisek wants to merge 15 commits into
OSGeo:mainfrom
gureckyfrantisek:issue-7212
Open

wxGUI/gmodeler: Correct variable replacement in model_convert.py#7241
gureckyfrantisek wants to merge 15 commits into
OSGeo:mainfrom
gureckyfrantisek:issue-7212

Conversation

@gureckyfrantisek

Copy link
Copy Markdown
Contributor

I found the issue which caused the previous variable in the same string not get recognized.

The issue was that when another match was found, the old value got replaced instead of the updated one called "parametrizedValue".

Also made the curly braces around variable names optional, they get recognized correctly but leaving them mandatory should be readable, that is up for debate.

I tested this with r.in.pdal and a *.laz file and the import worked as expected with both the values replaced with options.

This will resolve #7212.

@github-actions github-actions Bot added GUI wxGUI related Python Related code is in Python labels Mar 30, 2026
@gureckyfrantisek gureckyfrantisek changed the title fix: Correct variable replacement wxGUI/gmodeler: Correct variable replacement in model_convert.py Mar 31, 2026
@landam landam added this to the 8.5.0 milestone Apr 7, 2026
@landam landam requested review from landam and pesekon2 April 7, 2026 06:18
Comment thread gui/wxpython/gmodeler/model_convert.py
@landam landam added the bug Something isn't working label Apr 15, 2026
@gureckyfrantisek

Copy link
Copy Markdown
Contributor Author

I realized that the regex now allows for a variable to be parametrized as %{tile.laz or %tile}.laz, which works but could be unwanted behavior.

Should I improve the regex based on this realization?

@landam

landam commented Apr 23, 2026

Copy link
Copy Markdown
Member

ealized that the regex now allows for a variable to be parametrized as %{tile.laz or %tile}.laz, which

@gureckyfrantisek Yes, please refine the regex—either adopt curly brace syntax (%{tile}), which is recommended in GRASS 8.5 and newer, or retain the older format (%tile) for compatibility with GRASS 8.4. Nothing between.

@landam

landam commented May 6, 2026

Copy link
Copy Markdown
Member

I tested both models (v1: without braces and v2: with braces), see
06_grass_chm_v3.zip

Run model:

  • v1: works
  • v2: works

Display result:

  • v1: doesn't work
Failed to run command 'd.rast map=%tile_chm'. Details:                          
                                                                                
GRASS_INFO_ERROR(89749,1): Raster map <%tile_chm> not found                     
GRASS_INFO_END(89749,1)  
  • v2: doesn't work (see comment above)

Convert to Python script and run:

  • v1: works
  • v2: works

@landam

landam commented May 6, 2026

Copy link
Copy Markdown
Member

@gureckyfrantisek Just a note: it would be nice to define the regex for variables in a single place and reuse it across the code to avoid duplication. This isn’t a blocker for this PR, as the duplication already exists in the codebase.

@wenzeslaus wenzeslaus modified the milestones: 8.5.0, 8.6.0 May 8, 2026
@gureckyfrantisek

Copy link
Copy Markdown
Contributor Author

The issue is deeper, running the model now displays the maps correctly, but so should the maps when the Display prop is changed in canvas aswell as the map name after running the converted Python script.

I used a very simillar logic for all these cases but only after running the model it is substituted correctly.

Running converted Python doesn't fail but also doesn't display the maps and the Display prop in canvas fails.

@landam

landam commented Jun 12, 2026

Copy link
Copy Markdown
Member

The issue is deeper, running the model now displays the maps correctly, but so should the maps when the Display prop is changed in canvas aswell as the map name after running the converted Python script.

I can confirm that the Modeler now displays maps correctly. I also tested running the Modeler from the command line (g.gui.gmodeler), where I encountered the following error:

Traceback (most recent call last):
  File "https://github.com/home/martin/src/grass/dist.x86_64-pc-linux-gnu/gui/wxpython/gmodeler/panels.py", line 427, in OnModelDone
    layers = self._giface.GetLayerList().GetLayersByName(data.GetResolvedValue(resolved))
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'list' object has no attribute 'GetLayersByName'

Running converted Python doesn't fail but also doesn't display the maps and the Display prop in canvas fails.

A Python script (which was converted from a model) is not expected to add any maps to the Display.

Comment thread gui/wxpython/gmodeler/model.py Outdated
Comment thread gui/wxpython/gmodeler/model.py Outdated
Comment thread gui/wxpython/gmodeler/model.py Outdated
Comment thread gui/wxpython/gmodeler/model.py Outdated
Comment thread gui/wxpython/gmodeler/model_items.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/model.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment thread gui/wxpython/gmodeler/canvas.py Outdated
Comment thread gui/wxpython/gmodeler/model.py
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
Comment thread gui/wxpython/gmodeler/panels.py Outdated
@gureckyfrantisek

Copy link
Copy Markdown
Contributor Author

I applied and verified changes from code review, variable replacement now works as expected in my opinion.

@gureckyfrantisek gureckyfrantisek requested a review from landam June 18, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working GUI wxGUI related Python Related code is in Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Graphical Modeler: multiple variables used in single option are not substituted properly

4 participants