iTwin / appui

Monorepo for iTwin.js AppUi
MIT License
8 stars 2 forks source link

clearHideIsolateEmphasizeElements icon doesnt clear up when the element's visibility is toggled from Models tree #264

Closed muthubentley closed 1 year ago

muthubentley commented 1 year ago

Describe the bug clearHideIsolateEmphasizeElements icon doesnt clear up when the element's visibility is toggled from Models tree

To Reproduce Steps to reproduce the behavior: Issue can be reproduced in the below sandbox: https://www.itwinjs.org/sandboxes/MuthuKalamani/ContextToolWithModelTree

  1. Select and element and click "Hide Element" icon image
  2. Confirm the element is hidden in viewer and tree. clearHideIsolateEmphasizeElements eye icon(encircled in red) is also displayed in the top toolbar image
  3. Now press the eye icon in the tree to toggle the visibility of the element. Though all the elements are visible , clearHideIsolateEmphasizeElements eye icon doesnt clear up. image

Expected behavior When all elements are visible, clearHideIsolateEmphasizeElements eye icon should clear up from the viewer.

raplemie commented 1 year ago

This will be investigated for 4.1 release

muthubentley commented 1 year ago

@raplemie thanks. Can this be released before this month end?

raplemie commented 1 year ago

We dont have a date for 4.1 yet. It may come to a patch version to 4.0 (so earlier than 4.1) but I cant promise a release date for now.

muthubentley commented 1 year ago

@raplemie Understood, I will keep tracking the bug.

muthubentley commented 1 year ago

@raplemie @AntanasBuk Its been a while since this issue was created. Can you please prioritize this issue?

AntanasBuk commented 1 year ago

Yes, finally got around to this issue, will be looking into it next week

AntanasBuk commented 1 year ago

@muthubentley I remember looking into it a month back and found some other strange behaviors stemming from this issue so we will probably need to communicate what the intended behavior should look like (I'll compile a list later on).

muthubentley commented 1 year ago

@AntanasBuk Yes it is complicated than it looks. please let me know if we need to discuss the list of impacts.

AntanasBuk commented 1 year ago

Update:

Ok, so the list of issues is as follows:

  1. The original issue described above.
  2. Hiding an element through the Model Tree does not make clearHideIsolateEmphasizeElements icon appear.
  3. Hiding an element through the Model Tree (and in turn unselecting that element in the viewer) does not make hideElements icon disappear.

The problems with these issues are caused on our end by not adding listeners for hide/show elements events of the viewport and mostly only relying on buttons being clicked.

I've already managed to fix issues 1 and 3, so if nothing else comes up, I should be able to make a PR some time next week.

AntanasBuk commented 1 year ago

Just found another problem, hiding elements via Hide Categories button from StandardContentToolsUiItemsProvider and then showing them again via Model Tree completely prevents that element category from being hidden again via Hide Categories button. Guess this will not be a simple fix after all.

muthubentley commented 1 year ago

Just found another problem, hiding elements via Hide Categories button from StandardContentToolsUiItemsProvider and then showing them again via Model Tree completely prevents that element category from being hidden again via Hide Categories button. Guess this will not be a simple fix after all.

@AntanasBuk Is this one relevant to current issue? This seems to be a different problem related to trees

AntanasBuk commented 1 year ago

Its more of the same, we store element visibility override states based on appui events and ignore events from itwin-core. I am searching for a convenient way to check if any models and categories of elements visibility are being overriden (checking for overriden individual elements as described in your issue is a more simple case though and if you are in a hurry I'll definitely do a PR this week for that).

muthubentley commented 1 year ago

@AntanasBuk there is no hurry. Fukui also would like to have a stable version without impacts. Please take your time and analyze all the possible impacts.

AntanasBuk commented 1 year ago

https://github.com/iTwin/appui/pull/425

This PR fixes following issues and will be available with the next minor release:

  1. The original issue described above.
  2. Hiding an element through the Model Tree does not make clearHideIsolateEmphasizeElements icon appear.
  3. Hiding an element through the Model Tree (and in turn unselecting that element in the viewer) does not make.

Issues related to overriding categories and models to the best of my knowledge would require running queries constantly and that would be less than ideal. AppUI team will need to decide how to best approach these issues.

On another note, @muthubentley please tell us if this solves the issues you were facing. We'll keep this issue open and wait for your feedback :)

muthubentley commented 1 year ago

@AntanasBuk Thanks for the fix. Am out of office for an event this week. Will test and inform you the results by next week.

muthubentley commented 1 year ago

@AntanasBuk can you please let me know the version in which this fix is available?

AntanasBuk commented 1 year ago

@muthubentley the fix will be available with the next minor release (4.4.0). It should be available next week or at the end of this week.

raplemie commented 1 year ago

Hi @muthubentley, the fix for this was released today in 4.4.0. Let us know if this fixes your issue!

muthubentley commented 1 year ago

@AntanasBuk @raplemie Thanks for the fixes. Below three scenarios works well:

  1. The original issue described above.
  2. Hiding an element through the Model Tree does not make clearHideIsolateEmphasizeElements icon appear.
  3. Hiding an element through the Model Tree (and in turn unselecting that element in the viewer) does not make hideElements icon disappear.

But when the category is hidden from Tree, eye icon doesnt show up. I will check with partner about the behavior. Based on their reply we can handle it as a separate issue. This issue can be closed.