Skip to content
This repository was archived by the owner on Apr 23, 2025. It is now read-only.

fix: show 'no url/binding' node, use async leafstate for component (#687) #697

Merged
merged 2 commits into from
Feb 14, 2024
Merged

fix: show 'no url/binding' node, use async leafstate for component (#687) #697

merged 2 commits into from
Feb 14, 2024

Conversation

adietish
Copy link
Contributor

@adietish adietish commented Feb 2, 2024

fixes #687

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 2, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@adietish adietish self-assigned this Feb 2, 2024
adietish
adietish commented Feb 8, 2024
View reviewed changes
...main/java/org/jboss/tools/intellij/openshift/tree/application/ApplicationsTreeStructure.java
@@ -94,9 +94,7 @@ public Object getApplicationsRoot() {

@Override
public @NotNull LeafState getLeafState(@NotNull Object element) {
if (element instanceof ComponentNode) {
return LeafState.ALWAYS;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the fix: the leafstate for ComponentNode is now async

adietish
adietish commented Feb 8, 2024
View reviewed changes
...main/java/org/jboss/tools/intellij/openshift/tree/application/ApplicationsTreeStructure.java Outdated
@@ -318,6 +329,11 @@ public Object getParentElement(@NotNull Object element) {
public void commit() {
}

@Override
public boolean isToBuildChildrenInBackground(@NotNull Object element) {
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enhancement: retrieve children in a background thread (instead of UI thread)

@adietish adietish marked this pull request as ready for review February 8, 2024 14:28
@openshift-ci openshift-ci bot requested a review from sbouchet February 8, 2024 14:28
@sbouchet
Copy link
Collaborator

sbouchet commented Feb 9, 2024

@adietish : your enhancement are good idea, but may need more than 'just' displaying no url/url.
letme explain.
URL are actually shown only when running on dev mode, because they are 'active' and the context action opens a webbrowser to that address.
If your idea is to show 'no URL', it shouldbe clear taht it's no ACTIVE url that can be browsed, because when you describe the component using odo, it always shows URL taht are listed on the devfile.
What i propose is to display more information.

  • NO URL only when there is no URL in the devfile
  • URL in grey state, when there is URL in the devfile but the compoent is not running
  • URL in White/Green state, when the URL is active

this is somehow similar to what odo does, and we can rely on it to parse the state ( try out with descibe commands) .

WDYT ?

@adietish
Copy link
Contributor Author

@sbouchet: I suggest that I file your suggestions to an enhancement request and keep this PR focused on the fix, solely. Agree?

@adietish adietish force-pushed the issue-687 branch 2 times, most recently from 3b39ede to 0e423cb Compare February 13, 2024 13:11
@adietish adietish mentioned this pull request Feb 13, 2024
Open
@adietish
Copy link
Contributor Author

filed #697 (comment) to #707

@sbouchet
Copy link
Collaborator

thanks for filling a new enhancement request.

sbouchet
sbouchet approved these changes Feb 13, 2024
View reviewed changes
Copy link
Collaborator

@sbouchet sbouchet left a comment

Choose a reason for hiding this comment

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

LGTM. please follow up with #707

@openshift-ci openshift-ci bot added the lgtm label Feb 13, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbouchet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the lgtm label Feb 13, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 13, 2024

New changes are detected. LGTM label has been removed.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@adietish
Copy link
Contributor Author

/override ci/prow/e2e-openshift

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 14, 2024

@adietish: Overrode contexts on behalf of adietish: ci/prow/e2e-openshift

In response to this:

/override ci/prow/e2e-openshift

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@adietish adietish added the lgtm label Feb 14, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 933522b into redhat-developer:main Feb 14, 2024
@adietish adietish deleted the issue-687 branch February 14, 2024 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@sbouchet sbouchet sbouchet approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@adietish adietish

@sbouchet sbouchet

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URL aren't displayed anymore when component is on dev mode
2 participants
@adietish @sbouchet