RFR: JMC-5473: Quick search for Automated Analysis Result Table
Jie Kang
jkang at redhat.com
Tue Sep 17 21:20:27 UTC 2019
On Tue, Sep 3, 2019 at 2:57 PM Alex Macdonald <almacdon at redhat.com> wrote:
>
> On Wed, Aug 28, 2019 at 11:36 AM Jie Kang <jkang at redhat.com> wrote:
>>
>> On Wed, Aug 28, 2019 at 10:46 AM Jie Kang <jkang at redhat.com> wrote:
>> >
>> > Hi all,
>> >
>> > Carmine has requested I continue this work in his stead. Please find
>> > an updated webrev for review below.
>> >
>> > Webrev: http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.00/
>> > Bug: https://bugs.openjdk.java.net/browse/JMC-5473
>>
>> Ah; I had a mistake in the previous webrev (the TableViewer was no
>> longer set), please find an updated one below. Apologies for the
>> mistake.
>>
>> http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.01/
>>
>>
>
> Looks good - the functionality works as it should and the uitests are passing on both my Linux & Windows machines.
>
> There is an unused import that can be cleaned up at:
>
> ResultTableUi.java
> - unused import GridData @ line 50
Hi Alex,
Nice catch thanks. I've removed the unused import.
>
> And my only other nit is that the test is placed in "org.openjdk.jmc.flightrecorder.uitest.overview" which would be a new package that would only have this test inside of it. It doesn't test (yet at least) the entire Automated Analysis page so maybe it doesn't fit under "[..].flightrecorder.uitest.pages", but maybe it belongs under [..].flightrecorder.uitest alongside the other tests? Just a thought.
Sure; I've moved it to be under o.o.jmc.flightrecorder.uitest. Here's
the updated webrev:
http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.02/
How does it look?
Regards,
>
>>
>> Regards,
>>
>> >
>> >
>> > Regards
>> >
>> >
>> >
>> > On Thu, Aug 1, 2019 at 10:24 AM Carmine Vincenzo Russo
>> > <carusso at redhat.com> wrote:
>> > >
>> > > Hi Jie,
>> > >
>> > > Thanks for pointing out these errors, I'll fix them and get back with an updated version.
>> > >
>> > > Thanks,
>> > > Carmine
>> > >
>> > > On Thu, Aug 1, 2019 at 4:16 PM Jie Kang <jkang at redhat.com> wrote:
>> > >>
>> > >> On Thu, Aug 1, 2019 at 6:53 AM Carmine Vincenzo Russo
>> > >> <carusso at redhat.com> wrote:
>> > >> >
>> > >> > Hi,
>> > >> >
>> > >> > The attached patch addresses the issue JMC-5473[0] in which a search
>> > >> > feature for the Automated Analysis Result Table was lacking.
>> > >> >
>> > >> > In ResultOverview.java, I added the text component and a simple layout to
>> > >> > give the page more consistency when the table is displayed.
>> > >> > I also produced ResultOverviewTest.java to test if the table has
>> > >> > components, the search feature operates, and two more to check if the
>> > >> > behaviour is what we expect with a nonsense search and a specific search.
>> > >> >
>> > >> > A side note, since there were no tests for the Automated Analysis Result, I
>> > >> > had to add the tab in JfrUi and allow the record analysis in two other
>> > >> > classes, OldRecordingsVerificationTest.java and JfrRecordingTest.java,
>> > >> > because they go through all the tabs listed in JfrUi during their tests.
>> > >> >
>> > >> > How does it look?
>> > >>
>> > >> Hi Carmine,
>> > >>
>> > >> Thanks for doing this; the tests are appreciated! I have some issues
>> > >> to address below:
>> > >>
>> > >> diff --git a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/overview/R
>> > >> esultOverview.java
>> > >> b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/ov
>> > >> erview/ResultOverview.java
>> > >> --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/overview/ResultOv
>> > >> erview.java
>> > >> +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/overview/ResultOv
>> > >> erview.java
>> > >> [...]
>> > >> -
>> > >> +
>> > >>
>> > >> * Whitespace additions like the example shown above should be removed;
>> > >> there are others in the patch.
>> > >>
>> > >> - Map<Result, DataPageDescriptor> map = createResultMap();
>> > >> + GridLayout layout = new GridLayout(1, true);
>> > >> + this.form.getBody().setLayout(layout);
>> > >> + text = new Text(form.getBody(), SWT.BORDER |
>> > >> SWT.HORIZONTAL | SWT.SEARCH | SWT.RESIZE);
>> > >> + text.setLayoutData(new GridData(SWT.FILL,
>> > >> SWT.DEFAULT, true, false));
>> > >> + text.setMessage(Messages.ResultOverview_SEARCH_TABLE);
>> > >> + text.setToolTipText(Messages.ResultOverview_SEARCH_BAR);
>> > >> + Map<Result, DataPageDescriptor> map =
>> > >> createResultMap(text.getText());
>> > >> table = new ResultTableUi(form, toolkit,
>> > >> editor, loadedState, map);
>> > >> + ModifyListener listener = new ModifyListener() {
>> > >> + @Override
>> > >> + public void modifyText(ModifyEvent e) {
>> > >> + Map<Result,
>> > >> DataPageDescriptor> map = createResultMap(text.getText());
>> > >> + table.updateInput(map);
>> > >> + }
>> > >> + };
>> > >> + text.addModifyListener(listener);
>> > >> }
>> > >> * The additions to the Table UI done above should occur in the Table
>> > >> UI code, rather than in the overview which is managing the HTML UI and
>> > >> the Table UI, ie. the above should be within ResultTableUi.java
>> > >>
>> > >> * When switching to the Table and then back to the HTML, the resulting
>> > >> view is strange; see the linked image [1]. This may have to do with
>> > >> how the form's layout is modified by your addition. This form is the
>> > >> parent for the ResultTableUi. I would add children to it with custom
>> > >> layouts, but I would not modify the 'parent' layout itself.
>> > >>
>> > >> https://imgur.com/a/nS1m2T2
>> > >>
>> > >>
>> > >> Regards,
>> > >>
>> > >> >
>> > >> > Regards,
>> > >> > Carmine
>> > >> >
>> > >> > [0] https://bugs.openjdk.java.net/browse/JMC-5473
>> > >> >
>> > >> > --
>> > >> > Carmine Vincenzo Russo
>> > >
>> > >
>> > >
>> > > --
>> > > Carmine Vincenzo Russo
More information about the jmc-dev
mailing list