FLO-2DSoftware / qgis-flo-2d-plugin

A plugin for pre-processing/post-processing FLO-2D models
5 stars 7 forks source link

Why the settings from 'Select components from shapefile layer' dialog are not saved when the QGIS project exists/closes.? #1305

Closed FLO-2DJJ closed 1 month ago

FLO-2DJJ commented 2 months ago

The Advanced Settings stored in group 'FLO-2D' for the layers and fields selected in the 'Select components from shapefile layer' dialog, are saved when the dialog is closed. That's OK.

imagen

The problem is that when a different project is loaded the settings for this functionality are the same as the ones of the previous project.

It seems that the problem is when the project is exited/closed. At that moment the settings for 'Select components from shapefile layer' should be stored with the database.

rpachaly commented 1 month ago

Hi JJ,

Can you provide a gpkg that shows this bug?

FLO-2DKaren commented 1 month ago

I don't know how important this is but since JJ showed this, I also wonder if it is wise to have so many of our settings outside the FLO-2D group.

image

FLO-2DJJ commented 1 month ago

Those settings are old and not used. They should be deleted. They equivalent ones, now used, are the ones inside the FLO-2D group.

FLO-2DKaren commented 1 month ago

I'm not sure about that. This profile isn't that old.

FLO-2DJJ commented 1 month ago

Hi Robson,

This is a gpkg that show the problem:

SD Pumps May 05 D.zip

And a second project with other shapefile values for the Select components from shapefile layer dialog.

Auto assign nodes to links May X.zip

To see the issue you need to:

  1. Open that first project.
  2. See the shapefile assignments in Select components from shapefile layer. In this example only the Conduits and Pumps have the conduit name selected.
  3. Open a different project with other shapefiles selected.
  4. Save it.
  5. Open the first project. The original shapefile settings are missing in the Select components from shapefile layer. In the Advanced Settings the values are from the second project.
FLO-2DJJ commented 1 month ago

I'm not sure about that. This profile isn't that old.

Sorry @FLO-2DKaren,

I meant the ones starting with "sf_"

FLO-2DKaren commented 1 month ago

OK I see

rpachaly commented 1 month ago

Ok, I see what is happening here. The values are stored in the settings and every project that saves the "Select components from shapefile layer" updates the settings. When the first project is opened again, the settings are assigned to the other project.

My opinion, is it the better option to save this information on the settings? I would not save it to the settings but use a for loop with an if statement to check if the layer contains that field. Something like:

 lyr = self.data['user_swmm_conduits']["qlyr"] -> this should be the layer that is populated on the "Select inlets/junctions..."
 lyr_fields = lyr.fields()
 for index, field in enumerate(lyr_fields):
         field_name = field.name()
         if "length" in field_name: 
                  self.conduit_length_FieldCbo.setCurrentIndex(index)
         if "name" in field_name: 
                  self.conduit_length_FieldCbo.setCurrentIndex(index)
         ... 

This way we don't need to add a lot of stuff to the settings. And if the user layer does not contain the correct field name, then he'll have to select it manually.

What are your thoughts?

FLO-2DKaren commented 1 month ago

Another thing we could do is save those variables to a geopackage table and then they would stay with the project.

rpachaly commented 1 month ago

Another thing we could do is save those variables to a geopackage table and then they would stay with the project.

That's also a good idea. I prefer this then adding data to the settings.

FLO-2DJJ commented 1 month ago

@rpachaly, @FLO-2DKaren,

Before we proceed with any of the suggestions, I have a further question: When I switch from one project to another, the shapefile settings (the ones starting with "sf_" in the FLO-2D block, inside the blue rectangle) remain the same, while others change as expected (inside the red rectangle):

One project:

imagen

The other project:

imagen

Interestingly, this also occurs with other settings.

I’ve examined the code but couldn’t find any clues about this situation. Could this be related to the process of closing and opening each project?

rpachaly commented 1 month ago

Hi JJ,

Yes, you are correct. The lastGdsDir and lastGds are updated every time a project is opened or saved.

FLO-2DKaren commented 1 month ago

@FLO-2DJJ @FLO-2DNoemi @rpachaly

The first part of this video is off topic but I'd like JJ to see it and Noemi to approve it. Noemi, I show how I'd like the widget to look and I wanted your feedback.

The second part of the video is the data management idea. It is the topic that goes with this Issue. If Noemi approves this, Robson will implement this method. https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/assets/20424460/603d60a7-9bea-45f8-99fe-a603bf7f0166

rpachaly commented 1 month ago

A little feedback from me:

Import/Export SWMM should be on the toolbar (Inside the FLO-2D Import/Export button and right above Import RAS Geometry).

I agree with Karen's methodology of saving the fields on the geopackage. However, since a new table will be created. This must be done before the release.

FLO-2DKaren commented 1 month ago

Good idea about the import export. Noemi is going to give us feedback too.

FLO-2DNoemi commented 1 month ago

I like the idea of import-export buttons being moved to the toolbar. I agree with Karen that the import -export icons are not appropriate and we need to change them.

We need to avoid building large widgets. We have issues with large widgets in laptops. Shrinking the SD widget is a good idea. Select Components from shapefile layer can be rearranged in a smaller icon and auto assign too.

Saving SD data to the geopackage seems to be a good idea. I do not think this will impact the size of the geopackage. Please check how the size of the file increases by saving all these data to it.

We need to be careful in those particular cases where the data is saved to the geopackage and the user brings a new set of shapefiles modified outside the QGIS. The previous data that was saved needs to be completely replaced by the new data from the modified set of shapefiles when ASSIGN SELECTED FIELDS is done.

FLO-2DKaren commented 1 month ago

The icons were my fault. I thought they would look cool but they are inappropriate.

Saving the SD data won't grow the geopackage by much. It's the elevation data that is the problem child.

Removing those layers is not very intuitive but we can outline the process in the tutorials and the user manual. I'll add that as an issue on the documentation repo.

FLO-2DNoemi commented 1 month ago

Sounds good, let's move forward with these updates. The only one that is a concern is the removal of layers, we need to document this well as you mentioned.

FLO-2DJJ commented 1 month ago
  1. The idea of moving the import/export .INP to the external toolbar is attractive and can be done, but has an important complication: the two methods that do that functionality feed from other functions (methods) and variables (attributes) defined within the Storm Drain Widget python class. As I said, it can be done, but it would require some effort. I would rather move them to the icons bar (as a dropdown list), as Karen suggested. That would be really easy and immediate.
  2. Select components from shapefile layer and Auto-assign link nodes can also easily be moved to the Storm Drain Editor icons bar.
FLO-2DJJ commented 1 month ago

@rpachaly,

I need your help here.

Could you please give me some indications on how to save/retrieve the shapefile names and fields to/from the geopackage when loading/saving the Select components from shapefile layer dialog?. I need to replace the current code that saves/loads those settings from the FLO-2D group of the QGIS Advanced Settings. Karen suggests creating a table inside the geopackage to do it.

FLO-2DKaren commented 1 month ago

I want Robson to do this because we need a quick turnaround and he already knows how to do all of this.

JJ will you please help me with documentation? I will be behind once Robson moves the import export buttons. I will send you a MSWord doc that you can update. We can discuss that on the Documentation Repo.

rpachaly commented 1 month ago

Ok! I'll do it.

FLO-2DJJ commented 1 month ago

Wait until I do a pull request with the latest changes to the Storm Drain Widget. I'll do it now.

rpachaly commented 1 month ago

Sounds good! Let me know when it is done.

FLO-2DJJ commented 1 month ago

The pull request #1304 now has this new look for the Storm Drain Widget, with buttons for the Import INP, Export INP, Select components from shapefile, and Auto-assign (the icons of the buttons must be changed later):

imagen

@FLO-2DKaren, we'll replace the icons once you have the replacements.

Please review the pull request.

rpachaly commented 1 month ago

JJ, I'm going to merge this branch and review it as we go.

FLO-2DJJ commented 1 month ago

@rpachaly,

In the pull request #1304 you just merged, you can see the code to make the changes to save/retrieve the shapefile names and fields to/from the geopackage when loading/saving the Select components from shapefile layer dialog.

Look at class StormDrainShapefile. Particularly the methods:

_setup_layers_comboxes save_storm_drain_shapefile_field_names restore_storm_drain_shapefilefields

Please tell me if you need anything else.

(Ufff! I just spotted several wrong instances of _clear_all_inletattributes , that I left while making the last changes before deciding to go to the geopackage, but won't affect your work)

FLO-2DJJ commented 1 month ago

JJ will you please help me with documentation? I will be behind once Robson moves the import export buttons. I will send you a MSWord doc that you can update. We can discuss that on the Documentation Repo.

@FLO-2DKaren,

Yes Karen, just tell me what you want me to do.

rpachaly commented 1 month ago

It is done on https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/pull/1306

Here is a video explaining what I did: https://flo-2d.sharefile.com/d-s56c903703dd04d1899a6a64cf8e3d84e

(I messed up my microphone volume, it seems that I'm using a ham radio to record the video)

FLO-2DJJ commented 1 month ago

Hi @rpachaly,

I saw your video. I like what you've done very much. The intelligent code is very useful, we should keep it. A Nice addition.

I'm going to test the code to see if there is something to improve or fix. But from the video it looks perfect. Nice job.

FLO-2DJJ commented 1 month ago

Fixes #1305