-
Notifications
You must be signed in to change notification settings - Fork 174
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
Added toggle for filtering failed tasks #1325
base: master
Are you sure you want to change the base?
Added toggle for filtering failed tasks #1325
Conversation
This commit introduces a toggle action to filter out successful tasks from the execution page and expand all failed tasks.
@@ -12,6 +12,7 @@ Title_Gradle_Build_Script_Compare=Gradle Build Script Compare | |||
|
|||
Button_Label_Browse=Browse... | |||
|
|||
Action_FilterFailedTasks_Tooltip=Filter Failed Tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌
Action_FilterFailedTasks_Tooltip=Filter Failed Tasks | |
Action_FilterFailedTasks_Tooltip=Expand Failures |
@@ -23,6 +23,7 @@ public final class UiMessages extends NLS { | |||
|
|||
public static String Button_Label_Browse; | |||
|
|||
public static String Action_FilterFailedTasks_Tooltip; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name should be adjusted.
/** | ||
* | ||
*/ | ||
public final class ExpandAllFailedTasksAction extends Action implements SelectionSpecificAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public final class ExpandAllFailedTasksAction extends Action implements SelectionSpecificAction { | |
public final class ExpandFailuresAction extends Action implements SelectionSpecificAction { |
❌ the name doesn't correspond to the functionality. We collapse the tree and expand the nodes representing failed progress events.
|
||
this.contentProvider.toggleFilterFailedItems(); | ||
setChecked(this.contentProvider.isFilterFailedItemsEnabled()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setChecked(this.contentProvider.isFilterFailedItemsEnabled()); | ||
|
||
this.treeViewer.collapseAll(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.treeViewer.collapseAll(); | ||
|
||
Object rootObject = this.treeViewer.getInput(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -290,6 +291,9 @@ private void populateToolBar() { | |||
toolbarManager.appendToGroup(MultiPageView.PAGE_GROUP, new ExpandAllTreeNodesAction(getPageControl().getViewer())); | |||
toolbarManager.appendToGroup(MultiPageView.PAGE_GROUP, new CollapseAllTreeNodesAction(getPageControl().getViewer())); | |||
toolbarManager.appendToGroup(MultiPageView.PAGE_GROUP, new ShowFilterAction(getPageControl())); | |||
ExpandAllFailedTasksAction action = new ExpandAllFailedTasksAction(getPageControl().getViewer()); | |||
action.setContentProvider((ExecutionPageContentProvider)getPageControl().getViewer().getContentProvider()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ this can happen in the action's constructor. Then, you can use the same pattern as around, ie
toolbarManager.appendToGroup(MultiPageView.PAGE_GROUP, new ExpandAllFailedTasksAction(...));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add ExecutionPageContentProvider
as a constructor parameter, since UiContributionManager also takes an instance of the action and I don't have access to the content provider there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the looks of it, UiContributionManager
contributes actions to the tasks view
. In other words it's independent from the (execution) view this PR aims to improve. You probably don't need to modify it whatsoever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
} | ||
|
||
public void toggleFilterFailedItems() { | ||
this.filterFailedItems = !this.filterFailedItems; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Please don't do this. Use explicit getters and setters.
@@ -17,14 +21,24 @@ | |||
*/ | |||
public class ExecutionPageContentProvider implements ITreeContentProvider { | |||
|
|||
private boolean filterFailedItems = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ This is wrong as the state will get lost when the IDE restarts. Instead, store the state in WorkspaceConfiguration
. Then you can initialize the action from the worksace configuration and update it upon a toggle action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, the proper place to store the state is ExecutionViewState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ And the test coverage is missing. We should have something like ExecutionViewExpandAndCollapseAllTest
.
This commit introduces a toggle action to filter out successful tasks from the execution page and expand all failed tasks.
Fixes #970
Context
Previously, there was only the option to expand and collapse all tasks, making it harder to focus on failures. This change introduces a toggle action that, when enabled, hides successful tasks from the hierarchy in the execution page. When disabled, the full task hierarchy is restored.
This improvement makes it easier for users to debug failures without unnecessary distractions.