RFR: JDK-8302644: IGV: Apply filters per graph tab and not globally [v9]

[Christian Hagedorn]

On Wed, 29 Mar 2023 08:53:49 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:

>> In IGV the user can apply a set of filters to a graph. Currently, the same set of selected filters is applied to all graphs (globally). 
>> 
>> - With this change the use can define a set of filters for each individual graph tab using the `--Local--` profile
>> - Further a filter profile can be created that represents a set of filter. This filter profile can the be selected in each graph tab individually.
>> 
>> ### Global profile
>> Each tab has a  `--Global--` filter profile which is selected when opening a graph. Filters applied to the `--Global--` profile are applied to all tabs that have the `--Global--` profile selected. 
>> 
>> ### Local profile
>> Each tab has its own  `--Local--` filter profile. Filters applied to the `--Local--` profile are applied only to the currently selected tabs. Only one tab can be selected at a time and a tab gets selected by clicking on it. To make it more clear which tab is currently selected, the title of the selected tab is displayed in **bold** font.
>> <img width="1252" alt="tabA" src="https://user-images.githubusercontent.com/71546117/222127949-38a3ed8f-8f61-4485-aca6-67cbc341a4d1.png">
>> 
>> When clicking on a different tab with a different `--Local--` profile, the selected filters get updated accordingly.
>> <img width="1256" alt="tabB" src="https://user-images.githubusercontent.com/71546117/222127988-66be7a76-a638-4c40-b8f1-ad56a1ab1fd7.png">
>> 
>> ### New profile
>> The user can also create a new filter profile and give it a name. E.g. `My Filters`
>> <img width="396" alt="newProfile" src="https://user-images.githubusercontent.com/71546117/222128202-ea1f8de6-9260-4a78-b396-b69a354dc883.png">
>> 
>> The `My Filters` profile is then globally available to other tabs as well 
>> <img width="254" alt="selectProfile" src="https://user-images.githubusercontent.com/71546117/222128229-e33ab7b1-ebbe-4abc-a759-b50965ca64e1.png">
>> 
>> 
>> ### Filters for cloned tabs
>> When the user clones a tab, the `--Local--` profile gets cloned as well. Further the clone has the same filter profile selected when it gets opened
>> <img width="1250" alt="cloneTab" src="https://user-images.githubusercontent.com/71546117/222128014-2ca5eac0-ae8a-42da-b407-acc62c09edcf.png">
>> 
>> ### Saving of filters and profiles
>> When the users closes IGV, the filters (in their exact order) are save, as well as the filter profiles. The profile that was last used is selected when opening IGV.
>
> Tobias Holenstein has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix: update checkboxes when switching filter profiles
>   
>   based on feedback from @chhagedorn

That's a nice improvement! I've tried your patch out again and it works as expected. Thanks for fixing the issue discussed offline. I only have some minor code style comments. Otherwise, looks good to me!

src/utils/IdealGraphVisualizer/Filter/src/main/java/com/sun/hotspot/igv/filter/FilterChain.java line 133:

> 131:     public List<Filter> getFilters() {
> 132:         return Collections.unmodifiableList(filters);
> 133:     }

Missing new line
Suggestion:

    }

src/utils/IdealGraphVisualizer/FilterWindow/src/main/java/com/sun/hotspot/igv/filterwindow/FilterTopComponent.java line 352:

> 350:     }
> 351: 
> 352:     public void addFilter(CustomFilter cf) {

While at it, you could rename `cf` and `fo` to something more descriptive like `customFilter` and `fileObject`.

src/utils/IdealGraphVisualizer/Util/src/main/java/com/sun/hotspot/igv/util/RangeSliderModel.java line 49:

> 47: 
> 48:     public RangeSliderModel(RangeSliderModel model) {
> 49:         this();

I suggest to directly initialize the missing fields (`changedEvent` and `colorChangedEvent`) instead of initializing the other fields twice with `this()`.

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/DiagramViewModel.java line 163:

> 161:         showNodeHull = true;
> 162:         showEmptyBlocks = true;
> 163:         group = graph.getGroup();

You could keep that to make `group` `final` again and then just initialize it in `init()` (or `initGroup()`) by directly accessing the field.

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 379:

> 377:     }
> 378: 
> 379:     private boolean useBoldDisplayName = false;

Should be moved up to the other fields

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java line 386:

> 384:     }
> 385: 
> 386: 

Suggestion:

-------------

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/12714#pullrequestreview-1362595235
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151623131
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151627866
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151631346
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151637654
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151640971
PR Review Comment: https://git.openjdk.org/jdk/pull/12714#discussion_r1151641458