Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Features/typo symbol filtering #106

Merged
merged 22 commits into from
Mar 20, 2023

Conversation

robLittiere
Copy link
Contributor

@robLittiere robLittiere commented Mar 16, 2023

Hello,

We added a functionnality that allows the user to dynamically choose which pictogram they wish to display or to hide by checking a checkbox on the pictogram display menu.
Magrit will then only draw checked pictograms on the layer.

We made it as a quality of life improvement as we sometimes felt like we didnt to display all fields from our data but only selected ones. By default, all fields are selected and thus, we keep magrit's default behaviour

You can see a render exemple in the screenshot below

Capture d’écran du 2023-03-16 11-55-07

@mthh
Copy link
Member

mthh commented Mar 16, 2023

Hello @robLittiere

Thank you very much for this valuable contribution!

I glanced at the code and started to try this new feature. Overall it looks good but I noticed a few things:

  • pictograms that are not displayed on the map still appear in the legend, I think that this is not desirable,

  • this new feature has not been integrated into the code that saves the project as a project file, nor has it been integrated into the code that reloads the project file - in the client/js/map_project.js file), so currently when we try to reload a project with symbols filtered by your method, various error messages appear in the console and the reload operation fails.

Could you take care of these two points?
(the legend code and the project-file save/reload code are pretty messy, so feel free to ask if you need guidance or if you want me to take care of it)

armel and others added 6 commits March 16, 2023 16:11
so it's using generic magrit legend functions
Added filtered symbols to project save so we can retreive the information on project load
On project load, added logic to display only previously filtered symbols
Co-authored-by: ArmelVidali <vidali.armel@gmail.com>
@robLittiere
Copy link
Contributor Author

robLittiere commented Mar 16, 2023

Hello @mthh

Thank you for your quick feedback, indeed, we absolutly forgot about legend and project save/load ! Thank you for bringing this up.

What has been done after some time to understand how the project save/load flow work 😄

  • I modified the way we were filtering symbols so we actually filter the list of symbol_map that magrit uses to render its symbols. This way, Magrit actually generates the correct legend by itself.

  • I added the necessary so we save the list of desired filtered symbols upon saving a project whether it is saved manually or on local storage, I think it uses the same logic so both work.

  • Upon loading, we can get the previously saved list of filtered symbols so we can filter again when we project the TypoSymbol layer.

If other bugs are encountered, please let me know !

@mthh
Copy link
Member

mthh commented Mar 17, 2023

Thanks, I tested it and most of it seems to work.
Overall the code looks good to me and I see that you seem to have found your way around Magrit's code 😅

There is one last detail though before its OK to get merged.

The scenario is : the user selects his/her symbols before creating a layer (let's say we use the Brazil layer as in your example, and we deactivate 2 of the 5 categories - see my first screenshot) and then clicks on "confirmation", the window closes.

Capture du 2023-03-17 16-49-42

If the user reopens immediately the same symbol selection window, he/she will now only see the selected categories, the others have completely disappeared (see my second screenshot).

Capture du 2023-03-17 16-50-05

  • Ideally, the user should find the window as he/she left it (with the same categories deselected)
  • Otherwise, I'm also OK with the fact that the user finds the window in its initial state (all categories displayed).

@robLittiere
Copy link
Contributor Author

Thanks for your feedback, finding where to put the correct lines was not easy but it is done ! 👍

Your scenario is correct and is exactly what we imagined, however the issue you pointed is most definitly an unwanted behaviour, we would like at least to be able to check/uncheck all categories again. I'll see what I can do

We do this so it does not override the first symbols_map which contains all available fields for the category
Then changed the naming symbol_map to symbols_to_display in data_manager for legend projection, saving and loading project and reprojecting layers
@robLittiere
Copy link
Contributor Author

What has been done in the latest commit :

I gathered that we were actually ovewriting the symbol_map that was used to know which symbols should be showed or not. The problem was that Magrit uses this same map in "rendering_params" to let us know which fields are available for a category.
And so it created the problem where if you selected twice "choose symbols", the second time you would have only parts of the actually availabele fields.

In order to fix this, we have two solutions :

  • Add a field to rendering_params object that would only be used to list available fields for a category.

  • Don't modify the symbol_map variable and leave it as it is, although we would need to add another variable that contains the actual map of symbols to display.

Both solutions have the same effect, the first would maybe have been simpler to integrate.
As I could not find where the rendering_params were initially set or how it worked 😞 , I went with the second solution.

Which means I had to modify the naming variables in the legend, save/export/load code for projecting TypoSymbol layers. Instead of using the symbol_map property from the data_manager, we use its symbol_to_display property.

  • Ideally, the user should find the window as he/she left it (with the same categories deselected)
  • Otherwise, I'm also OK with the fact that the user finds the window in its initial state (all categories displayed).

For now, I have left it as default behaviour, when you click on "choose symbols" again, you will get everything checked.
To make it better, indeed we could save a state of the checkboxes in the rendering_params (since its used for the displaybox) or elsewhere) to which fields were checked or unchecked. We will try to take the time to implement it for sure !

@mthh
Copy link
Member

mthh commented Mar 17, 2023

Thanks a lot !
I pulled your last changes and I played a bit with the new functionality, that's great 👍

I will finish to review it and merge it on Monday.

@robLittiere
Copy link
Contributor Author

Alright, thanks for taking the time !

Let us know if there are further changes to make 👍

@mthh
Copy link
Member

mthh commented Mar 20, 2023

I took the liberty to push to your branch :

  • to fix a minor bug when category values were number (you could trigger it by using the layer "Communes du Grand Paris" and asking for a Typo Symbols representation on the field "DEPARTEMENT", if one ore more categories were filtered an error was encountered).
  • to fix some formatting issues
  • to change where the filtering of features to be rendered happens (in order to avoid adding empty <image> elements as in the image below to the SVG for the features that dont need a symbol)

empty_image

I will probably merge it later today. Thanks a lot for your contribution !

@robLittiere
Copy link
Contributor Author

robLittiere commented Mar 20, 2023

Hello @mthh

I have checked the changes and commits you made.

Thank you for fixing the the bugs you found.

Indeed, having all those empty images in the DOM was a mistake, thanks for fixing it.
You probably saw my big comment stating that it would be a better solution but I was scared of breaking something if we removed the whole objects 🤣

Thank you for your feedback, don't hesitate to come back to me or @ArmelVidali (who also contributed) for further improvements 👍

@mthh mthh merged commit 46c28bb into riatelab:master Mar 20, 2023
@mthh mthh added this to the 0.14.0 milestone Mar 23, 2023
@mthh
Copy link
Member

mthh commented Mar 24, 2023

I released version 0.14.0 with those changes. It will be made available during the night on https://magrit.cnrs.fr/ server.

@ArmelVidali
Copy link
Contributor

Great !

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

Successfully merging this pull request may close these issues.

None yet

3 participants