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

[Alex Macdonald]

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

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.


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