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

feat: add refresh to the WU context menu #388

Merged

Conversation

dehilsterlexis
Copy link
Contributor

fixes #386

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review. I removed passing the element to the refresh function so that when the user explicitly selects refresh, it forces a fresh. The reason being the way it is currently, the only time it refreshes is when it receives an event that is local. because it checks for any changes in the local tree. If you are listing workunits that are from other people that has changed, the refresh currently will not refresh because it is checking for a local change. I think the user expects refresh to force a refresh without condition.

Copy link
Member

@GordonSmith GordonSmith left a comment

Choose a reason for hiding this comment

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

What happens when the user clicks refresh in the title area?
What happens when the user right clicks on a WU and selects refresh?

@ddehilster
Copy link

@GordonSmith they both force a refresh. They call the same function. The httpsystems-refresh command is only called in those two places, so it won't change the behavior of other refresh calls.

@GordonSmith
Copy link
Member

The one in the title should refresh the entire tree, while the one a WU should just refresh that node?

@ddehilster
Copy link

That makes sense. I will look into a way to pass the element without it checking if that element has changed. I need to force that one element. Will work on that.

@dehilsterlexis
Copy link
Contributor Author

@GordonSmith the refresh forces an entire refresh only if the user instantiates the refresh manually. For review.

userRefresh(): void {
this.updateMenu();
super.refresh();
}
Copy link
Member

Choose a reason for hiding this comment

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

This method isn't needed, you can just call refresh()?

Choose a reason for hiding this comment

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

Actually, that would work!

Copy link
Member

Choose a reason for hiding this comment

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

The whole userRefresh method is not needed

Copy link
Contributor Author

@dehilsterlexis dehilsterlexis Feb 28, 2024

Choose a reason for hiding this comment

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

Yes it is. If you call refresh from package.json, the refresh that is attached to the wu treeview item will send the item and it will check if the item has changed, and will not force the refresh.

@ddehilster
Copy link

@GordonSmith for review

userRefresh(): void {
this.updateMenu();
super.refresh();
}
Copy link
Member

Choose a reason for hiding this comment

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

The whole userRefresh method is not needed

@dehilsterlexis dehilsterlexis force-pushed the ADD_WU_CONTEXT_REFRESH branch from 47c8ba8 to 664d53a Compare March 4, 2024 14:49
@dehilsterlexis
Copy link
Contributor Author

@GordonSmith for review

@GordonSmith GordonSmith merged commit 3bcc7f7 into hpcc-systems:trunk Mar 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add refresh to WU context menu
3 participants