[PATCH] JMC-4740: There should be a better default column ordering for event attributes
Guru
guru.hb at oracle.com
Tue Aug 21 12:30:09 UTC 2018
I am waiting for One more review from Sharath / Jay, Post to that I will push the changes.
Thanks,
Guru
> On 21-Aug-2018, at 5:01 PM, Miro Wengner <miro.wengner at gmail.com> wrote:
>
> Hi Guru,
> thank you!
> should I do anything more according to this fix ?
Nope (Unless there is any mismatch with your earlier .diff sent over email).
>
> Kind regards,
> Miro
>
>> On Aug 21, 2018, at 12:28 PM, Guru <guru.hb at oracle.com> wrote:
>>
>> Hi Marcus & Miro,
>>
>> I have rebased the diff, Reviewed and tested the use case mentioned in JBS.
>> Webrev : http://cr.openjdk.java.net/~ghb/ext/miro/JMC-4740/webrev.0/
>> Changes looks good to me.
>>
>> Thanks,
>> Guru
>>> On 13-Aug-2018, at 9:13 PM, Marcus Hirt <marcus.hirt at oracle.com> wrote:
>>>
>>> I will sponsor this change, but I think someone else should review it, as I
>>> helped with it.
>>>
>>> Guru, Sharath?
>>>
>>> Kind regards,
>>> Marcus
>>>
>>> On 2018-08-13, 17:25, "jmc-dev on behalf of Miro Wengner" <jmc-dev-bounces at openjdk.java.net on behalf of miro.wengner at gmail.com> wrote:
>>>
>>> Hi Everyone,
>>> here is my patch for a review,
>>> Thank you Marcus!
>>>
>>> Kind Regards,
>>> Miro
>>>
>>>
>>> diff -r 2cc768144a86 application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java
>>> --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java Fri Aug 10 14:03:47 2018 +0530
>>> +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java Mon Aug 13 15:37:03 2018 +0200
>>> @@ -38,6 +38,7 @@
>>> import java.util.ArrayList;
>>> import java.util.Arrays;
>>> import java.util.Collection;
>>> +import java.util.Collections;
>>> import java.util.Comparator;
>>> import java.util.HashMap;
>>> import java.util.HashSet;
>>> @@ -145,6 +146,7 @@
>>> import org.openjdk.jmc.ui.charts.XYQuantities;
>>> import org.openjdk.jmc.ui.column.ColumnMenusFactory;
>>> import org.openjdk.jmc.ui.column.TableSettings;
>>> +import org.openjdk.jmc.ui.column.TableSettings.ColumnSettings;
>>> import org.openjdk.jmc.ui.handlers.ActionToolkit;
>>> import org.openjdk.jmc.ui.handlers.MCContextMenuManager;
>>> import org.openjdk.jmc.ui.misc.ChartCanvas;
>>> @@ -176,17 +178,25 @@
>>> };
>>> }
>>>
>>> - private static Map<String, Color> fieldColorMap = new HashMap<>();
>>> + private static final Map<String, Color> FIELD_COLOR_MAP = new HashMap<>();
>>> + private static final Map<String, Integer> DEFAULT_COLUMNS_ORDER;
>>>
>>> static {
>>>
>>> // FIXME: Create FieldAppearance class, similar to TypeAppearence?
>>> - fieldColorMap.put(JdkAttributes.MACHINE_TOTAL.getIdentifier(), new Color(255, 128, 0));
>>> - fieldColorMap.put(JdkAttributes.JVM_SYSTEM.getIdentifier(), new Color(128, 128, 128));
>>> - fieldColorMap.put(JdkAttributes.JVM_USER.getIdentifier(), new Color(0, 0, 255));
>>> - fieldColorMap.put(JdkAttributes.JVM_TOTAL.getIdentifier(), new Color(64, 64, 191));
>>> + FIELD_COLOR_MAP.put(JdkAttributes.MACHINE_TOTAL.getIdentifier(), new Color(255, 128, 0));
>>> + FIELD_COLOR_MAP.put(JdkAttributes.JVM_SYSTEM.getIdentifier(), new Color(128, 128, 128));
>>> + FIELD_COLOR_MAP.put(JdkAttributes.JVM_USER.getIdentifier(), new Color(0, 0, 255));
>>> + FIELD_COLOR_MAP.put(JdkAttributes.JVM_TOTAL.getIdentifier(), new Color(64, 64, 191));
>>>
>>> // FIXME: Handle ColorProvider and combined events
>>> + Map<String, Integer> columnsOrderMap = new HashMap<>();
>>> + columnsOrderMap.put(createColumnId(JfrAttributes.START_TIME), 1);
>>> + columnsOrderMap.put(createColumnId(JfrAttributes.DURATION), 2);
>>> + columnsOrderMap.put(createColumnId(JfrAttributes.END_TIME), 3);
>>> + columnsOrderMap.put(createColumnId(JfrAttributes.EVENT_THREAD), 4);
>>> + DEFAULT_COLUMNS_ORDER = Collections.unmodifiableMap(columnsOrderMap);
>>> +
>>> }
>>>
>>> public static final Color ALLOCATION_COLOR = new Color(64, 144, 230);
>>> @@ -198,7 +208,7 @@
>>> public static final String RESULT_ACTION_ID = "resultAction"; //$NON-NLS-1$
>>>
>>> public static Color getFieldColor(String fieldId) {
>>> - return fieldColorMap.getOrDefault(fieldId, ColorToolkit.getDistinguishableColor(fieldId));
>>> + return FIELD_COLOR_MAP.getOrDefault(fieldId, ColorToolkit.getDistinguishableColor(fieldId));
>>> }
>>>
>>> public static Color getFieldColor(IAttribute<?> attribute) {
>>> @@ -1053,4 +1063,33 @@
>>> && JfrAttributes.START_TIME.getAccessor(type) != JfrAttributes.END_TIME.getAccessor(type);
>>> }
>>>
>>> + public static TableSettings createTableSettingsByOrderByAndColumnsWithDefaultOrdering(final String orderBy, final Collection<ColumnSettings> columns) {
>>> + final Stream<ColumnSettings> defaultOrderColumns = columns.stream()
>>> + .filter(c -> DEFAULT_COLUMNS_ORDER.containsKey(c.getId()))
>>> + .filter(c -> !c.isHidden())
>>> + .sorted((c1, c2) ->
>>> + Integer.compare(DEFAULT_COLUMNS_ORDER.get(c1.getId()),
>>> + DEFAULT_COLUMNS_ORDER.get(c2.getId()))
>>> + );
>>> + final Stream<ColumnSettings> naturalOrderColumns = columns.stream()
>>> + .filter(c -> !DEFAULT_COLUMNS_ORDER.containsKey(c.getId()))
>>> + .sorted((c1, c2) -> String.CASE_INSENSITIVE_ORDER.compare(c1.getId(), c2.getId()));
>>> + final List<ColumnSettings> resultColumns = Stream.concat(defaultOrderColumns, naturalOrderColumns).collect(Collectors.toList());
>>> + return new TableSettings(orderBy, resultColumns);
>>> + }
>>> +
>>> + public static TableSettings createTableSettingsByAllAndVisibleColumns(final Collection<String> allColumns, final Collection<String> visibleColumns) {
>>> + final List<ColumnSettings> defaultListCols = new ArrayList<>();
>>> + for (String columnId : allColumns) {
>>> + defaultListCols.add(new ColumnSettings(columnId, !visibleColumns.contains(columnId), null, null));
>>> + }
>>> + return createTableSettingsByOrderByAndColumnsWithDefaultOrdering(null, defaultListCols);
>>> + }
>>> +
>>> + private static String createColumnId(IAttribute<?> attr) {
>>> + return new StringBuilder().append(attr.getIdentifier())
>>> + .append(":")
>>> + .append(attr.getContentType().getIdentifier())
>>> + .toString();
>>> + }
>>> }
>>>
>>>
>>>
>>>
>>> diff -r 2cc768144a86 application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/EventBrowserPage.java
>>> --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/EventBrowserPage.java Fri Aug 10 14:03:47 2018 +0530
>>> +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/EventBrowserPage.java Mon Aug 13 15:37:38 2018 +0200
>>> @@ -335,7 +335,8 @@
>>> Composite parent = oldListControl.getParent();
>>> oldListControl.dispose();
>>> list = DataPageToolkit.createSimpleItemList(parent, itemListBuilder, container,
>>> - new TableSettings(orderBy, listColumns), Messages.EventBrowserPage_EVENT_BROWSER_SELECTION);
>>> + DataPageToolkit.createTableSettingsByOrderByAndColumnsWithDefaultOrdering(orderBy, listColumns),
>>> + Messages.EventBrowserPage_EVENT_BROWSER_SELECTION);
>>> parent.layout();
>>> list.show(filteredItems);
>>> }
>>>
>>>
>>>
>>> diff -r 2cc768144a86 application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/itemhandler/ItemListAndChart.java
>>> --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/itemhandler/ItemListAndChart.java Fri Aug 10 14:03:47 2018 +0530
>>> +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/pages/itemhandler/ItemListAndChart.java Mon Aug 13 15:38:01 2018 +0200
>>> @@ -67,6 +67,7 @@
>>> import org.openjdk.jmc.flightrecorder.ui.IPageContainer;
>>> import org.openjdk.jmc.flightrecorder.ui.ItemCollectionToolkit;
>>> import org.openjdk.jmc.flightrecorder.ui.StreamModel;
>>> +import org.openjdk.jmc.flightrecorder.ui.common.DataPageToolkit;
>>> import org.openjdk.jmc.flightrecorder.ui.common.FilterComponent;
>>> import org.openjdk.jmc.flightrecorder.ui.common.ImageConstants;
>>> import org.openjdk.jmc.flightrecorder.ui.common.ItemHistogram.HistogramSelection;
>>> @@ -153,10 +154,11 @@
>>>
>>> // FIXME: Should we use the state here, if the columns have been updated?
>>> // FIXME: Should we change the column state if the user explicitly has configured the columns?
>>> - TableSettings itemListSettings = TableSettings.forStateAndColumns(
>>> - state != null ? state.getChild(LIST_SETTINGS) : null, acc.getAllAttributes().keySet(),
>>> - acc.getCommonAttributes().keySet());
>>> -
>>> + final TableSettings itemListSettings = state == null ? DataPageToolkit.createTableSettingsByAllAndVisibleColumns(
>>> + acc.getAllAttributes().keySet(), acc.getCommonAttributes().keySet()) :
>>> + TableSettings.forStateAndColumns(state.getChild(LIST_SETTINGS), acc.getAllAttributes().keySet(),
>>> + acc.getCommonAttributes().keySet());
>>> +
>>> Composite listComposite = toolkit.createComposite(tabFolder);
>>> listComposite.setLayout(GridLayoutFactory.swtDefaults().create());
>>> itemList = itemListBuilder.buildWithoutBorder(listComposite, itemListSettings);
>>>
>>>
>>>
>>>
>>
>
More information about the jmc-dev
mailing list