Integrate browser based views in ViewContainer #359
Conversation
taldekar
changed the base branch from
taldekar/AssetProviders
to
feature/viewRefactor
| public boolean hasFailed() { | ||
| return this == FAILED; | ||
| } | ||
|
|
||
| public boolean isActive() { | ||
| return this == ACTIVE; | ||
| } | ||
|
|
||
| public boolean isPending() { | ||
| return this == PENDING; | ||
| } |
There was a problem hiding this comment.
Unless we anticipate these checks becoming more dynamic I don't see the value.
There was a problem hiding this comment.
Sounds good, I'll make the change.
| Optional<String> content = getContent(); | ||
| Activator.getEventBroker().post(WebViewAssetState.class, | ||
| content.isPresent() ? WebViewAssetState.RESOLVED : WebViewAssetState.DEPENDENCY_MISSING); |
There was a problem hiding this comment.
It is typically bad practice to have constructors performing business logic beyond basic instantiation and member assignment. These seem like tasks suited for an explicit initializer method. That seems like something that can be pushed to the base class since the login webview is doing the same thing.
There was a problem hiding this comment.
Makes sense. I'll move it to an initializer method. Would I call this method from whatever class is using the asset provider. Also, this logic is updated in the latest change, so it would be difficult to move this to the base class after #361.
| @@ -105,16 +96,20 @@ private void updateChildView() { | |||
| layout.topControl = newViewComposite; | |||
| parentComposite.layout(true, true); | |||
|
|
|||
| activeView = newView; | |||
| currentView = newView; | |||
| containerLock.unlock(); | |||
There was a problem hiding this comment.
Use caution here: if for any reason this code is never reached (for instance, an exception is thrown executing the logic in this critical section) then you will end up deadlocking the component. Good practice is for this code to run in a try block and the unlock in a finally.
There was a problem hiding this comment.
Yup, that makes sense. I didn't think about that. I'll make the change.
| public Composite setupView(final Composite parent) { | ||
| super.setupView(parent); | ||
|
|
||
| setupParentBackground(parent); | ||
| var result = setupBrowser(parent); | ||
| // if setup of amazon q view fails due to missing webview dependency, switch to | ||
| // that view | ||
| // and don't setup rest of the content | ||
|
|
||
| if (!result) { | ||
| showDependencyMissingView("update"); | ||
| return; | ||
| return parent; | ||
| } | ||
| var browser = getBrowser(); | ||
|
|
||
| addFocusListener(parent, browser); | ||
|
|
There was a problem hiding this comment.
A lot of this logic can probably also be moved to the base class.
There was a problem hiding this comment.
I'll look into this.
* Pass populated AWS_CA_BUNDLE env var to Flare (#341) * Add UI notification to alert user of deprecated manifest version (#312) * Revert commit 'Add UI notification to alert user of deprecated manifest version' * Revert 'Pass populated AWS_CA_BUNDLE env var to Flare' * Rebase changes from Browser Provider PR * Fix ViewRouter * Integrate browser based views in ViewContainer * Fix checkstyle issues * Fix AmazonQBrowserProvider tests * Fix bug due to display sync exec call * Add semaphore locking to container to prevent race conditions * Add browser focus handling * Add state checking methods to LspState * Clean up code * Remove unnecessary parent assignment from chat webview * Refactor ViewVisibilityManager to default to one view * Add accidentally removed telemetry emissions * Update plugin descriptor (#360) * Move semaphore locking/unlocking to try/finally block --------- Co-authored-by: Jonathan Breedlove <[email protected]> Co-authored-by: Nicolas <[email protected]>
Issue #336
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.