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

Implement Quick Access feature in LayerSwitcher #1437

Closed
wants to merge 132 commits into from

Conversation

linusfj
Copy link
Member

@linusfj linusfj commented Nov 24, 2023

This pull request introduces the Quick Access feature and additional enhancements as described in issue #1380.

Key features implemented include:

  • Quick Access grouping in LayerSwitcher for access to frequently used layers.
  • Filter functionality in LayerSwitcher for efficient layer management.
  • Improved Snackbar notifications for layer toggling actions.
  • Enhanced layer visibility indicators in LayerSwitcher.
  • Improved Draw Order tab functionality for managing layer drawing order.
  • Section for Quick Access in the admin UI.

Closes #1237, #1257, #1284, #1296, #1300, #1347, #1365, and #1380.

dennisbergstrom and others added 30 commits December 14, 2022 11:20
Resolved an issue where toggling a parent group's visibility did not accurately reflect the visibility of its sublayers.

The `setGroupHidden` and `setGroupVisible` functions were updated to maintain the correct state of `visibleSubLayers`.
The following features have been added in this commit:

SnackbarProvider.js:
- Create `SnackbarContext` to share snackbar state and controls among components.
- Implement `SnackbarProvider` component to set up `SnackbarContext` provider using the `useSnackbar` hook from `notistack` for snackbar controls.
- Initialize shared state (`messageItems`) to hold list of messages for snackbar display.
- Utilize `NotistackSnackbarProvider` to render the snackbar and provide the shared state and functions through `SnackbarContext`, enabling access to snackbar controls and shared state in child components.

useSnackbar.js:
- Create `useSnackbar` custom hook to manage snackbar messages with methods for adding (`addToSnackbar`), removing (`removeFromSnackbar`), hiding (`hideSnackbar`), displaying snackbar messages without storing (`displaySnackbar`) and clearing all messages (`clearAllMessages`).
- Implement functionality to handle group and layer specific snackbar messages for visibility notification of layers at different zoom levels.
- Include handling for non-grouped messages, displaying individual messages as standalone notifications.
- Add a close action button to each snackbar message and enable automatic control of snackbar visibility duration.

App.js:
- Update `SnackbarProvider` import statement to use the `SnackbarProvider` component.
Implement a queue-based system to manage state updates in the `useSnackbar` function to solve race conditions.

Changes include:
1. Create a new `updateQueue` ref to hold all state update operations.
2. Adjust `addToSnackbar` and `removeFromSnackbar` to push their state update functions into `updateQueue`, rather than executing them directly.
3. Add a new function `processQueue` that takes the next update function from the queue and executes it.
4. Call `processQueue` in `addToSnackbar` and `removeFromSnackbar` after their update is pushed into the queue, starting the processing of queued updates.
1. Import and implement `useSnackbar` custom hook for managing notifications. This enables addition, removal, and display of Snackbar messages.
2. Update the `listenToZoomChange` and `zoomEndHandler` functions to effectively handle zoom level changes and visibility of the layer at the current zoom level.
3. Refactor render logic to adjust CheckBoxIcon's color based on layer visibility, type, and zoom level.
Changes include:
1. Import and implement `useSnackbar` custom hook in GroupLayer for handling notifications.
2. Use a custom hook within a `useEffect` to remove layer captions from the Snackbar message when the visibility of sublayers in a group changes.
3. Update the `handleLayerItemClick` method in GroupLayer to manage visibility toggling of group layers and remove the appropriate layer captions from the Snackbar message.
4. Pass `zoomVisible` state as a prop from GroupLayer to SubLayerItem component to ensure consistent layer visibility management.
5. Modify the `getLayerToggleIcon` function in SubLayerItem to adjust the icon color based on the zoom visibility.
Import `useSnackbar` in MapCleaner to use `clearAllMessages` that clears all Snackbar messages.
Changes include:
- Add features that allow users to create new `Quick Layers` by inputting data into fields.
- Ability to add and delete layers.
- Import and validate JSON data.
- Automatically generate unique IDs for each `Quick Layer`.
- All `Quick Layers` are displayed in a list.
- Filter feature to search through the list of `Quick Layers`.
- A success message is displayed if the save or import was successful, and an error message is shown if the save or import failed.
Add new classes for managing Quick Layers in the backend.

Changes include:
`LayerMenuOptions.cs`: New property `quickLayersPresets` to store a list of `Quick Layer` presets.
`QuickLayer.cs`: This class defines the structure of a `Quick Layer`.
`QuickLayerMetadata.cs`: This class holds metadata about a `Quick Layer`.
`QuickLayerPreset.cs`: This class represents a `Quick Layer` preset.
Optimize performance by utilizing `useCallback` and update `useEffect` dependencies to resolve ESLint errors.
Add default values to various configuration settings in `simpleMapConfig.json`. A notable addition is an example for the `quickLayersPresets`.
@linusfj
Copy link
Member Author

linusfj commented Jan 22, 2024

The bugs related to the default configurations should now be resolved, and updated examples have been committed.

@jacobwod, could you check and confirm? Specifically, are you still encountering the issue with your configuration after attempting to re-save all configuration files in the admin UI?

@jacobwod
Copy link
Member

@linusfj, it's good to see that you've made changes to the config files. (However, it seems that you've introduced unresolved conflicts in your recent commit.)

However, significant bugs and the slowdown in overall UI responsiveness introduced in this PR still remain, as far as I can tell.

Given these facts, this PR is not ready for merge yet, which we also discussed last week.

I hope that you understand that given the state of this PR, I currently see no point in testing it thoroughly again.

@jacobwod
Copy link
Member

Converted PR to draft to reflect the current state:

  • functionality isn't ready for merge to develop, as discussed in a meeting last week
  • PR branch contains unresolved conflicts

@jacobwod jacobwod marked this pull request as draft January 23, 2024 08:57
This commit improves the LayerSwitcher's behavior by introducing auto-expansion of groups during filtering and ensuring a clean and organized view when the filter is cleared.

Changes made:
- Enabled auto-expansion of parent groups when a filter match is found within nested layers, improving the visibility of search results.
- Optimized filter handling by ensuring groups that contain matched layers are expanded during filtering.
- Introduced a reset mechanism to collapse all groups and clear layer filtering states when the filter input is cleared, preventing the display of an unnecessarily long and cluttered list.
- Enabled detailed view toggle for system layers.
- Ensured CQL filter and Quick Access button are conditionally displayed based on layer type.
- Improved code robustness for handling undefined properties in related components.
@linusfj
Copy link
Member Author

linusfj commented Jan 25, 2024

Here is a summary of the current status:

The current method for saving Quick Layers involves using the "Add to Favorites" option, which utilizes localStorage for storage. The saved layers can subsequently be accessed by loading them. If you still want to proceed with the implementation as you've described, it could be considered a feature request.

Resolved issues:

  • ESLint errors have been addressed.
  • Auto-expansion functionality for groups during filtering has been added.
  • The error message associated with anchorEl in the layer details view has been resolved.
  • Configuration-related issues have been addressed. Example configurations have been updated to align with the new functionality in LayerSwitcher.
  • The toggle for the detailed view of system layers has been enabled.

Unresolved issues:

  • CQL filtering has not been thoroughly tested with a functioning layer. Feedback from someone actively using the filter would be highly beneficial.
  • The issue with anchorEl that occurs when pressing the "Themes" button still persists. A temporary workaround that addresses the error is to avoid using the Tooltip component on the button.

@Hallbergs
Copy link
Member

Here is a summary of the current status:

The current method for saving Quick Layers involves using the "Add to Favorites" option, which utilizes localStorage for storage. The saved layers can subsequently be accessed by loading them. If you still want to proceed with the implementation as you've described, it could be considered a feature request.

Resolved issues:

  • ESLint errors have been addressed.
  • Auto-expansion functionality for groups during filtering has been added.
  • The error message associated with anchorEl in the layer details view has been resolved.
  • Configuration-related issues have been addressed. Example configurations have been updated to align with the new functionality in LayerSwitcher.
  • The toggle for the detailed view of system layers has been enabled.

Unresolved issues:

  • CQL filtering has not been thoroughly tested with a functioning layer. Feedback from someone actively using the filter would be highly beneficial.
  • The issue with anchorEl that occurs when pressing the "Themes" button still persists. A temporary workaround that addresses the error is to avoid using the Tooltip component on the button.

Thanks for the summary and the resolved issues!

An addition to the unresolved issues:

  • Rendering performance issues. As shown in the last code group meeting, the layer-switcher becomes way to slow in maps with +300 (ish) layers. We'll have to look into the possibility of changing how the layer-items are rendered to increase performance.

I am looking forward to test-results from other Hajk-organizations.

- Converted LayerGroup back from React.Component to React.PureComponent.
- Resolved an issue where clearing the filter did not correctly reset the list of layers and groups. This was due to the state not being updated properly in the case of nested structures (groups, layers, sublayers).
- Enhanced the `resetFilterStatus` method to ensure proper resetting of the filter status. Now, it recursively marks each node as filtered (`isFiltered = true`) and collapsed (`isExpanded = false`). This ensures that the filter status is correctly propagated to all child components.
- Updated the `handleFilterValueChange` method to create a new layer tree structure before resetting the filter status. This change involves creating a shallow copy of each top-level node and applying `resetFilterStatus` to update the filter status throughout the tree.
@maan002
Copy link
Contributor

maan002 commented Feb 1, 2024

The performance in Sydnärke is acceptable with the current number of layers used in Hajk. But since the number of layers will increase the performance issue may show up when you least want it.

Two other suggestions:

  1. Make it possible in Admin to configure the number of characters that the user must enter before the filtering/searching begin. To force a filtering with less number of characters you should press "Enter".
  2. Perhaps add the possibility to the user to keep all layers in a group when the group name is hit by the filter. Now you will get an empty group.

@linusfj
Copy link
Member Author

linusfj commented Feb 1, 2024

I have addressed the rendering performance issues. By reverting to React.PureComponent and optimizing the state management methods resetFilterStatus and handleFilterValueChange, performance has been notably improved, especially in map configurations with many layers.

The 'information' tool, if present in the mapConfig, is recognized as a supported tool and not flagged as unsupported.

Closes #1118.
Adjust the padding-bottom property of an element to ensure the last list item in the background layers tab is fully visible.
Issue:
After loading a layer package (theme), all system layers were inadvertently removed from the map, which prevented them from being added again afterwards.

Solution:
The `resetVisibleLayers` function was updated to keep layers marked as 'system'. This adjustment ensures that system layers are not affected during the process of resetting layers.
@jacobwod
Copy link
Member

Intro

This is a follow-up to my initial review.

Findings

Warnings

On initial launch, console warnings still prevail.
Skärmavbild 2024-02-14 kl  13 44 39

The UI (fonts, line heights, more) seem "off"

Compare the new one:
Skärmavbild 2024-02-14 kl  13 50 10

With the current look:
Skärmavbild 2024-02-14 kl  13 50 23

I don't see why we should change the ratio between font size and line heights - the Material UI defaults are well-thought out and should remain.

Still blank details view

Skarminspelning.2024-02-14.kl.13.51.47.mov

Unacceptably many re-renderings when hovering a basic UI elements

This has been "felt" by many but I don't think we've done any actual tests. Here it is.

Current develop

Note how quickly the icon state changes. Also, the render time (in ms, in the right panel - 10 ms in the first render commit, 13 ms in the sixth). Also, there are no renderings related to any component of LayerSwitcher.

Skarminspelning.2024-02-14.kl.14.05.35.mov

This PR

The icon state is really sluggish (unthrottled machine). Render time (first and sixth render commit) is 20-30x the previous values. A lot of rendering related to LayerSwitcher occurs.

Skarminspelning.2024-02-14.kl.13.57.25.mov

Conclusion

It took me a considerable amount of time to write the first review. (It also took a while to prepare this one.)

As I've shown above, multiple issues from the first review still exist (both easy ones [like the warnings] and harder ones [like the performance issue]).

Given these facts, I stop here and will not continue with a full-fledged review. In my opinion, this code is far from being ready for a merge.

@jesade-vbg
Copy link
Contributor

Here is some things I noticed.

I can't scroll at all

no-overflow-scroll

Depth warning (One of the reasons it's laggy?)

depth-warning

Can toggle when nothing below

can-toggle-when empty

This commit splits the previously combined `useEffect` into separate hooks. It also addresses a critical performance issue where the component could enter an infinite rendering loop, leading to a "Maximum update depth exceeded" error.
This commit improves the filter logic within the `LayerSwitcherView component. Now, when a group's name matches the filter text, all layers and subgroups within that group are made visible, even if they do not individually match the filter text.

Key changes include:
- Filter matches both up and down the hierarchy, ensuring parent groups expand to reveal matching children and that all items within a matching group are shown.
- Updating the `filterTree` function to consider matches on parent nodes and apply visibility and expansion accordingly.
After loading a `Favorite`, all system layers were inadvertently removed from the map, which prevented them from being added again afterwards.
@Hallbergs Hallbergs closed this Mar 25, 2024
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.

7 participants