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

Explicitly mark UnsafeWidgetUtilities #9967

Merged
merged 1 commit into from
Aug 26, 2021
Merged

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Aug 26, 2021

What it does

Fixes #9960 by working around it.

This PR creates a new namespace for non-error-checked versions of Phosphor
utilities, replaces existing code blocks, and applies the new
utilities to cases that were throwing errors.

As the name implies, these utilities are unsafe because they
violate the expectations of Phosphor widgets. We should investigate
why we ever wrote code to do this in the first place, and fix it
if possible.

How to test

  1. Start the application.
  2. Open the console.
  3. Observe no errors about attachment or detachment.
  4. Hover over ViewContainerParts with toolbars.
  5. The toolbars should work.
  6. Use the tree search in the Navigator Widget
  7. You should see the search bar, in addition to highlights for the results.
  8. Dragging and dropping from the navigator tree into the main area should work.

The listener is set up onAfterAttach, so previously when an error was thrown, the listener might not be set up correctly.

Review checklist

Reminder for reviewers

Signed-off-by: Colin Grant colin.grant@ericsson.com

@vince-fugnitto vince-fugnitto added the quality issues related to code and application quality label Aug 26, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
I confirmed that:

  • we now properly reflect the unsafe utilities
  • dnd works well
  • toolbar items continue to work well

@alvsan09
Copy link
Contributor

Would you please re-base the change as there is a conflict with master

@colin-grant-work colin-grant-work force-pushed the bugfix/false-attachments branch from 561c6c5 to ed277d5 Compare August 26, 2021 16:39
@colin-grant-work
Copy link
Contributor Author

@alvsan09, thanks for the pointer. I've rebased.

Create a new namespace for non-error-checked versions of Phosphor
utilities, replaces existing code blocks, and applies the new
utilities to cases that were throwing errors.

As the name implies, these utilities are unsafe because they
violate the expectations of Phosphor widgets. We should investigate
why we ever wrote code to do this in the first place, and fix it
if possible.

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work force-pushed the bugfix/false-attachments branch from ed277d5 to d54c114 Compare August 26, 2021 17:22
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

I don't see any related errors and the functionality works as expected !
Thanks !!

@paul-marechal paul-marechal merged commit 43b7de8 into master Aug 26, 2021
@paul-marechal paul-marechal deleted the bugfix/false-attachments branch August 26, 2021 18:41
@github-actions github-actions bot added this to the 1.17.0 milestone Aug 26, 2021
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
Create a new namespace for non-error-checked versions of Phosphor
utilities, replaces existing code blocks, and applies the new
utilities to cases that were throwing errors.

As the name implies, these utilities are unsafe because they
violate the expectations of Phosphor widgets. We should investigate
why we ever wrote code to do this in the first place, and fix it
if possible.

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
Create a new namespace for non-error-checked versions of Phosphor
utilities, replaces existing code blocks, and applies the new
utilities to cases that were throwing errors.

As the name implies, these utilities are unsafe because they
violate the expectations of Phosphor widgets. We should investigate
why we ever wrote code to do this in the first place, and fix it
if possible.

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onAfterAttach being run when widget not attached
4 participants