RFR: JMC-5473: Quick search for Automated Analysis Result Table

Alex Macdonald almacdon at redhat.com
Wed Sep 18 14:58:59 UTC 2019 

On Tue, Sep 17, 2019 at 5:20 PM Jie Kang <jkang at redhat.com> wrote:

> 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:
>

Great, thanks. LGTM.


>
> 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