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

Enhancing User Session #10657 #10712

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rowheat02
Copy link
Contributor

@rowheat02 rowheat02 commented Dec 4, 2024

Description

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Enhancement

#10657

What is the current behavior?
Only able to save maps and featureGrid Configs and while clearing session all session used to get cleaned
#10657

What is the new behavior?

Other config like bookmarks, mapTemplates, userPlugins along with existing one gets saved and individual session can be cleared.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@rowheat02 rowheat02 marked this pull request as ready for review December 4, 2024 07:05
@rowheat02 rowheat02 self-assigned this Dec 4, 2024
@rowheat02 rowheat02 added this to the 2025.01.00 milestone Dec 4, 2024
@rowheat02 rowheat02 linked an issue Dec 4, 2024 that may be closed by this pull request
11 tasks
@tdipisa tdipisa requested a review from offtherailz December 13, 2024 13:50
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The icon of delete is the same of the delete map. It is a pre-existing issue, but is confusing.
    image
    I'd suggest clear-brus icon for user session clean (or a custom icon). Let's wait for a confirmation from @tdipisa about this.
    image

  • To be more clear I'd change the window as following:

    • Instead of simple "Waring", insert in the title "Clear user session"
    • The text should be: "Are you sure to remove the current user session? This will restore the original map/context configuration, removing your current changes. Please select what you want to remove and unselect what you want to preserve." (properly localizing strings).
    • A little more margin between text and tree, to better isolate the parts (or a lightgray border for the tree)
    • and button on bottom with normal size (they are giant).
      Bug:
  • Inconsistent selection after closing and re-opening the window (see the video, click on everything, then close and reopen, last items are selected). Same if I save the session.

screencast-localhost_8081-2025_01_14-17_32_07.webm
  • Visibility messed up on user session clean up/restore (see blue borders on layers, indicating the current seleted background. (Also checking-unchecking layers makes the background layer selection change). ALSO THIS IS PRE-EXISTING.

bug_background.webm
Here a screenshot on dev, same issue: image

  • Created a map with annotation and measure layer. Clean up session just to make sure. Remove measure layer and refresh. I have 2 annotation layers. Please fix this and test better these corner cases, adding unit tests to guarantee this issue not happens. NOTE: looks to be a pre-existing bug. It happens also on DEV. JFI @tdipisa
bug_sess_measure.mp4
```mermaid
graph TD
    
    S[Start] --> A
    A[Load Context] -->|Override Map Config| B[Load Map] 
    B --> |Override partially map config| C[Load Session]
    C --> |Save Session| C
    C --> |Clear config| D[Clear Session] 
    D --> |Reload| A
```
    

* @param {object} customHandlers - session Saved By registerCustomSaveHandler
* @returns {object} updated overrideConfig
*/
export const updateOverrideConfigToClean = (override, thingsToClear = [], originalConfig = {}, customHandlers = {}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some unit test alone.

@offtherailz
Copy link
Member

For both developers and testers, @rowheat02 MAKE YOU SURE YOU TEST SESSION:

  • Within a context ( #/context/user_session/)
  • Within a map ('#/context/user_session/1234)
  • With default viewer ('#/viewer/1234, tweaking localConfig to add by default UserSession plugin

This is to @ElenaGallo too, when this issue will be merged in next days.

@tdipisa
Copy link
Member

tdipisa commented Jan 15, 2025

@rowheat02 possibly we should try to address all reported points above. Just let me know how much time these require in your opinion, especially for pre-existing bugs: if the effort is too much we can consider opening a separated issue for them. We should possibly stay in 1pts for all requested changes above.

@offtherailz
Copy link
Member

@rowheat02 I added the doc update as discussed.
Please follow with all tasks except existing, then estimate existing and let's check together.

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

Successfully merging this pull request may close these issues.

Enhancing User Session
3 participants