RFR: 6677: search to highlight cells in the flame view
Alex Macdonald
aptmac at openjdk.java.net
Thu Feb 6 10:58:49 UTC 2020
On Sat, 1 Feb 2020 11:19:43 GMT, Miroslav Wengner <mwengner at openjdk.org> wrote:
> ticket: https://bugs.openjdk.java.net/browse/JMC-6677
> /contributor add Alex Macdonald [Alex.Mcdonald at redhat.com](mailto:Alex.Mcdonald at redhat.com)
Not sure if I'm supposed to be contributor on this PR, but my e-mail is `almacdon at redhat.com` regardless.
> conclusion:
>
> 1. resize works (on mac as I've no windows easy to access ;)
The svg stylesheet change works on Linux, and should also work on Windows.
> 2. search considers node and package name. It searches for the strings that contains desired sequence.
The search feels more intuitive now, thanks. Is it possible to have the search fire automatically (instead of hitting enter) when a certain amount of characters are entered, maybe 3+? I think there's a table of two in JMC which operate the same way.
There appears to be an edge case somewhere when searching for the string, but it's in the JavaScript so it's difficult to debug. But I'll type out my steps that what's happening.. maybe it has something to do with escaping characters:
- I have two entries of `ArrayList.add(Object [..])`.
- If I search for "add(", then no node is highlighted
- if I search for "add", then it highlights both those nodes
- Now that the nodes are highlighted, I can type anything in the search bar and it will not update the chart i.e., the two nodes remain highlighted.
- I would expect that inputting a string that cannot be found would highlight the nodes
Additionally, I found some formatting errors in the commits here .. but there's also quite a bit that made it through previously and it'd be nice to clean that up IMO. Do we have any JavaScript formatting guidelines that we're following?
With regards to the width comment on the closed https://github.com/openjdk/jmc/pull/44#issuecomment-580854588,
> I can also totally remove the width scale but I've observed some not wanted behaviour on the MacOS, have you tested it on Mac ?
What was the unwanted behaviour you were seeing?
Here's my initial feedback on the PR:
1. As for the sizing fix, I don't think setting the chart to 95% of the window size [[0]](https://github.com/openjdk/jmc/pull/45/files#diff-30491bb3d763f9e956043f3402596f31R88) is the complete fix; the problem is that the svg doesn't change it's size when the chart is changed. So if the chart increases in size it is cutoff by the svg, and if the chart decreases in size then it leaves excess white space.
GIF1: Increasing the chart size, but the SVG doesn't change
GIF2: Decreasing the chart size, but the SVG doesn't change
2. As for the search functionality itself, I think it looks really nice. My feedback for the moment is that it is using `startsWith()` for checking the strings, so it's only possible to search by class .. it would be neat to be able to search within the string as well. e.g., search for `.run()` and have it highlight all instances of `.run()` being called.
[0] https://github.com/openjdk/jmc/pull/45/files#diff-30491bb3d763f9e956043f3402596f31R88
[1] https://github.com/openjdk/jmc/pull/45/files#diff-c309933c5cd238df4805885ae811bae1R128
application/org.openjdk.jmc.flightrecorder.flameview/src/main/js/flameviewColoring.js line 120:
> 119: const colorCell = function (d) {
> 120: if (textToSearch !== "" && (evaluateSearchElement(d.data.p) || evaluateSearchElement(d.data.n))){
> 121: return "magenta";
formatting: space before `{`
application/org.openjdk.jmc.flightrecorder.flameview/src/main/js/flameviewColoring.js line 127:
> 126:
> 127: const evaluateSearchElement = function(text){
> 128: if((text !== undefined && text.toLowerCase().includes(textToSearch))){
formatting: space before `{`
application/org.openjdk.jmc.flightrecorder.flameview/src/main/js/flameviewColoring.js line 128:
> 127: const evaluateSearchElement = function(text){
> 128: if((text !== undefined && text.toLowerCase().includes(textToSearch))){
> 129: return true;
formatting: space after `if`
formatting: looks like there's an extra pair of brackets on the if-statement
formatting: space before {
application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 13:
> 12:
> 13: <body>
> 14: <div id="search" align="right">
out of curiosity, why the change from using `onresize` to adding a listener to the browser?
application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 61:
> 60: const inputText = packageInputField.value;
> 61: if(textToSearch !== inputText){
> 62: textToSearch = inputText.toLowerCase();
formatting: space after `if`
formatting: space before `{`
application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 29:
> 28: packageInputField.addEventListener("keyup", function(event) {
> 29: if(event.keyCode === 13){
> 30: executeSearch();
formatting: space after `if` and before `{`
application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 59:
> 58:
> 59: function executeSearch(){
> 60: const inputText = packageInputField.value;
formatting: space before `{`
application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/ImageConstants.java line 113:
> 112:
> 113: public static final String ICON_FLAMEVIEW_SEARCH = "magnifyer64.png"; //$NON-NLS-1$
> 114:
`s/magnifyer/magnifier/`, and the name of the `.fng` will have to be updated as well
-------------
PR: https://git.openjdk.java.net/jmc/pull/45
More information about the jmc-dev
mailing list