Community
Participate
Working Groups
This is a set of Trace Control View UI enhancements: new UI, support of most of trace-status fields, support for start and stop notes.
I3ae49dece2056c8e8fd9eb21118926b039a5cdf0
Gerrit Change-Id: I3ae49dece2056c8e8fd9eb21118926b039a5cdf0
Gerrit review at: https://git.eclipse.org/r/#/c/7997/
Hi Marc, I have added new patch https://git.eclipse.org/r/#/c/7997/. It is aligned with all gdb patches I have set and it is independent from -gdb-set patch/bug. It would be nice if you can tell something about it.
(In reply to comment #4) > Hi Marc, > I have added new patch https://git.eclipse.org/r/#/c/7997/. > It is aligned with all gdb patches I have set and it is independent from > -gdb-set patch/bug. > It would be nice if you can tell something about it. Thanks Dmitry, I have started to look at the proposed changes. First thing is that I get assertions. Please add -ea to your vmargs in the Arguments tab when launching your test Eclipse. Be careful that the -ea option must be in the lower box of arguments. The top box if for eclipse, the bottom is for the jvm and -ea is a jvm flag. Once that is enabled, you'll get assertions when you are illegally using some DSF calls outside the DSF executor. You should see some of these with the current patch :)
I didn't have a lot of time to test today but I thought I post some comments right away to get the ball rolling. I'll test more tomorrow. This is a great start, thank you! Before going into comments, let's start by saying that UI stuff is very hard to 'get right'; my comments below are my opinion and I would appreciate your view if you don't agree with me. I'm not in any way an expert in UI. 0- I actually found the original screenshot you sent me was nicer than the layout in the patch. The patch still uses too much text directly in the view (as the current implementation of the view does). Maybe using a visible textbox for the user name and notes would make things look more UI-like? 1- I think having the "Save Trace Data" button in the main toolbar could be confusing. Normally, the little disk icon indicates that the data of the view will be saved, which is not really the case here. 2- Not sure I like the gray background for the view :) It makes the toolbar blend too much into the view when the toolbar is too big to fit next to the view tab. Also, all other views have a while background for Debug, so it is probably better to keep it white. 3- When pressing the 'edit' button to input the user or the notes, I suggest that pressing 'Esc' will cancel the changes and revert back to the old one. 4- We need a different icon for the circular buffer, like a single arrow that goes full circle 5- Indentation of the buffer text seems a little more to the right than the rest. 6- Why are we polling GDB once the trace is started? We added the 'refresh' button to avoid polling. The user must be conscious that buffer size/tpCount collected cannot be updated live. Although it is nice to see the buffer fill up live... We could make polling an option, but we need to be able to turn it off. 7- During a trace experiment, we no longer see how many records have been collected. I think this info is easier to understand than the buffer size, so we should show both like before. 8- The slide bar for trace frames is causing delays. When I slide it back and forth it seemed to cause a lot of work to Eclipse and communication with GDB. How about we only trigger on a slide bar change when the user lets it go? 9- The slidebar for trace frames is not visible at the start and when tracing, however if I stop tracing it appears, and then if I start tracing again it remains. We should be consistent about displaying it or not during tracing.
Created attachment 223871 [details] The patch Hi Marc, this patch addresses issues 2-9 and intentionally skips 0 and 1. I want to keep them separate because UI discussion process is not as straigtforward as bugfixing. I still experience problems uploading patches to gerrit, sorry to disturb you, but it seems nobody cares about keeping gerrit working.
Then about 1): the reason I moved save from menu to toolbar was to show user that such feature exists at all, because it was not easy to find save before that. If you inisist I can rollback this part, but it seems to me that to be confused about save in toolbar is less ugly than fail to find it at all. WDYT?
And I have a question: icon for circular buffer is from here http://www.iconfinder.com/ajax/download/png/?id=25585&s=16 (Creative Commons (Attribution 3.0 Unported)), slightly modified by me. I failed to find something appropriate with EPL and I don't like two other self-made variants I included into the patch. SO I preper this derivation from CC licensed icon, but I'm not sure if it is appropriate.
Thanks for the new patch. I will have a look next week.
I've pushed the patch as patchset 3 on Gerrit. https://git.eclipse.org/r/#/c/7997/
Created attachment 223951 [details] Separate patch with UI experiments to be applied on top of previous patch. Separate patch with UI experiments to be applied on top of previous patch.
Created attachment 223952 [details] Screenshot to the second patch. Offline mode. Screenshot to the second patch. Offline mode.
Created attachment 223953 [details] Screenshot to the second patch. Live mode. Screenshot to the second patch. Live mode.
(In reply to comment #7) > I still experience problems uploading patches to gerrit, sorry to > disturb you, but it seems nobody cares about keeping gerrit working. Apparently, if you change the remote address for gerrit to use: https://git.eclipse.org/r/cdt/org.eclipse.cdt.git (notice the removed '/p')it will work. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=393735#c49 I've update the wiki to mention this new address: http://wiki.eclipse.org/CDT/git#Using_Gerrit_for_CDT
Sorry Dmitry but I haven't had time to look at this yet. I will plan it in my sprint right after the holiday vacation.
(In reply to comment #7) > Created attachment 223871 [details] > The patch > > Hi Marc, > this patch addresses issues 2-9 and intentionally skips 0 and 1. I want to > keep them separate because UI discussion process is not as straigtforward as > bugfixing. I still experience problems uploading patches to gerrit, sorry to > disturb you, but it seems nobody cares about keeping gerrit working. Sorry for the delay Dmitry. The patch I pushed for you didn't contain the icons so I'm getting NPEs. The icons are in the path attached to this bug, but I don't think I can extract them from that, you would have to attach them separately. Instead, can you push a new version of the patch to Gerrit which contains the icons? I assume you should be able to push to gerrit with the suggestion from comment 15. You can fetch 3 from gerrit, add the 7 icons, update the commit, then push again to gerrit. Thanks
(In reply to comment #17) > Instead, can you push a new version of the patch to Gerrit which contains > the icons? Glad to see that we returned to discussion, will do it asap.
I have posted comments in Gerrit. They mostly all pretty minor and should be easy to fix. I still have to review the one big file TraceControlView.java, but maybe we should discuss the UI details before spending time on that file. I've focused on the code, but I will now pay attention to the user experience and post comments about the UI itself. Thanks
Hi Marc, thank you for commnents, I have found my current development tree a bit different from the patch I have posted, there were another experiments how to improve UI based on discussion with Vladimir. I saw your comments about UI glitches you have found, do you have any other thoughts or wishes about it? I will try to cleanup patch based on list in gerrit and resend it. Thank you, Dmitry
(In reply to comment #20) > Hi Marc, thank you for commnents, I have found my current development tree a > bit different from the patch I have posted, there were another experiments > how to improve UI based on discussion with Vladimir. I'm looking forward to seeing what you propose. > I saw your comments > about UI glitches you have found, do you have any other thoughts or wishes > about it? I will try to cleanup patch based on list in gerrit and resend it. I'm not sure I mentioned this in Gerrit or not, but when connecting to a remote target, the trace notes are not fetched from the target or pushed to the target. What I mean is that when we connect I think we should: 1- check if the target has trace-notes, trace-user and trace-stop-notes set, and if it does, we should update the CDT view 2- if no notes are set on the target but we have some set in the view, we should push them to the target. As for the UI, I'm not thrilled with the way the notes are layed out. The label "User" and the string for the user all blend together and it looks like we just have a lot of text as we did before; same for the other notes and summary strings. I'd like us to have a more graphical view and less plain text. This can simply mean a different layout to make it less focused on the text. Another idea I had, although I'm not sure how difficult it would be, is to use a type of 'pie' chart to represent the buffer when we use the circular buffer. The background could be something like gray and fill up in a different color; once it is full, a single line would keep on turning as the buffer keeps writing, but the color would be all the same as the buffer is full. I thought this would represent the circular buffer case pretty well. What do you think?
Just a note that feature freeze is only a couple of months away. Let's try to get this in for this release.
Mark, I had a flu last week, this week I have scheduled 3 workdays for tracepoints UI.
(In reply to comment #23) > Mark, I had a flu last week, this week I have scheduled 3 workdays for > tracepoints UI. Ping. We only have 4 weeks left if we want to get this accepted for Kepler.
Created attachment 233275 [details] Proposed patch New patch, note that there is one class FlatButton.java stolen from our codebase, it is written by Vladimir Prus, and posted on his permission, but probably this require separate procedure. Please advice.
Created attachment 233276 [details] Proposed patch
Created attachment 233296 [details] Proposed patch Patch with proper signed off
Marc, are you planning to review this patch? Thanks!
(In reply to Vladimir Prus from comment #28) > Marc, > > are you planning to review this patch? > > Thanks! Woops, sorry. I lost track because it was never pushed to Gerrit. Did Dmitry ever get his gerrit account working?
(In reply to Marc Khouzam from comment #29) > Did Dmitry ever get his gerrit account working? It worked only once for me far ago.
I've pushed https://git.eclipse.org/r/#/c/18726/ on Dmitry's behalf, since he's away this week.
(In reply to Vladimir Prus from comment #31) > I've pushed https://git.eclipse.org/r/#/c/18726/ on Dmitry's behalf, since > he's away this week. Could you instead push the patch to the same review as before? Just amend your commit to use the gerrit changeId: Change-Id: I3ae49dece2056c8e8fd9eb21118926b039a5cdf0 You should also put Dmitry at the Author in the commit, no? I don't know if this will work for a non-committer, but if it does, you can abandon the new review. Thanks
Hi Marc, can you look at this change, it should have proper change ID https://git.eclipse.org/r/#/c/7997/ The issue with gerrit was that I tried to commit to commit owned by other person accordinf to cla management site. Now it supports multiple emails associated with one account and this fixed the problem.
(In reply to Dmitry Kozlov from comment #33) > Hi Marc, can you look at this change, it should have proper change ID > https://git.eclipse.org/r/#/c/7997/ Thank you. I have started the review. To go faster, I will make changes myself in the code for what I think does not need to be discussed. What needs discussion I will put as Gerrit comments. Once ready, I'll post a new version with my changes.
*** cdt git genie on behalf of Vladimir Prus *** Bug 390827 - Trace View enhancement Create necessary button. Signed-off-by: Vladimir Prus <vladimir@xxxxxxxxxxxxxxxx> [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=99b34b33524e01e19f71cd809c02f7d7122aeb26
I've reviewed the patch from a coding point-of-view, made some modifications myself (to go faster) and pushed to gerrit. The changes I made can be seen by comparing patchset 8 (Dmitry's patch, rebased), and patchset 9 (updated with my changes). I will now spend some time testing and looking at the UI in more detail.
Well, we've made it :) Patchset 15 is good to commit. Mikhail, since the contribution is > 250 lines, can you commit it for me since you work at the same company as Dmitry? Thanks! Thanks Dmitry for the great contribution!
Marc thank you for your patience and lots of ideas and reviews.
Patch reviewed and checked in.
Thanks Mikhail for committing. I've updated the N&N with this feature. Feel free to change what I put or add things: https://wiki.eclipse.org/CDT/User/NewIn84#Trace_Control_view_enhancements
