Bug 372032

Comment 1 Pawel Piech 2012-04-05 16:42:49 EDT

I posted a patch with fix to gerrit for Marc to review :-)

The patch simply retargets the commands to the top-level toolbar and adds a tester to the debug view toolbar contributions so they're shown/hidden in tandem with other run control actions.

However, when testing the actions I found their appearing/disappearing behavior rather bizarre.  Given that there's so much more room on the global toolbar, we may want to keep them visible as long as the action set is enabled. (just my 2c).

Comment 2 Marc Khouzam 2012-04-07 15:47:29 EDT

(In reply to comment #1)
> I posted a patch with fix to gerrit for Marc to review :-)

Thanks Pawel!  Comments in Gerrit.

> However, when testing the actions I found their appearing/disappearing behavior
> rather bizarre.  

Things are broken in 4.2.  I will do some tests and report bugs.  In 3.8, the behavior is as expected.

> Given that there's so much more room on the global toolbar, we
> may want to keep them visible as long as the action set is enabled. (just my
> 2c).

If we can't get things to work properly with 4.2, as they did in 3.8, then we may have no choice.  However, if possible, I find that hiding the buttons when not enabled, is less confusing to the user.  

What I had in mind was that users that sometimes use reverse debug, would keep the action set enabled all the time; this would only make the reverse toggle button visible.  Then, only when the user actually enables reverse debugging for the session (they must press the reverse toggle button), the reverse buttons would appear.

Remember that for GDB, if the user does not toggle reverse debugging for a session, reverse debugging won't be possible at all.  Also, currently, GDB does not support reverse debugging for non-stop or multi-threaded apps, so I prefer not to annoy the user with buttons that are very rarely used.

What do you think?

Comment 3 Marc Khouzam 2012-04-07 15:48:13 EDT

Gerrit review here:
https://git.eclipse.org/r/#/c/5535/

Comment 4 Marc Khouzam 2012-04-30 09:11:30 EDT

Ok, the problems of platform have been pretty much resolved for reverse debugging:  bug 376441 and bug 376442.

Does the current bug fall under 'new feature' and needs to be in by M7 this week?

Comment 5 Pawel Piech 2012-04-30 17:48:47 EDT

(In reply to comment #4)
I'd like to have this wrapped up now (m7).  I updated the placement of the actions to be the same as debug view and fixed the request evaluation logic.  I still left the refreshElement() in reverse toggle action on post execute, so that the toggle state of both actions stays in sync.

http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?h=bug372032&id=9e879482a1a76555021eb80b3e20ee66c75d9d2b

Comment 6 Marc Khouzam 2012-05-01 06:35:27 EDT

(In reply to comment #5)
> (In reply to comment #4)
> I'd like to have this wrapped up now (m7).  I updated the placement of the
> actions to be the same as debug view and fixed the request evaluation logic.  I
> still left the refreshElement() in reverse toggle action on post execute, so
> that the toggle state of both actions stays in sync.
> 
> http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?h=bug372032&id=9e879482a1a76555021eb80b3e20ee66c75d9d2b

Thanks Pawel for taking care of this.

However, the commit is identical as the code that was put in Gerrit.  I don't see the changes you mention above.  And the commit was made on a branch called 'bug372032' instead of master...

Comment 7 Marc Khouzam 2012-05-01 06:39:42 EDT

Oh, and if you don't do the commit through Gerrit, you'll have to abandon the review in Gerrit to get rid of it.

Comment 8 Pawel Piech 2012-05-01 09:57:25 EDT

Thanks Marc, I haven't had such a big miscommit with git in a while.  I think I somehow committed changes in one repo, then pushed another, and the branch which I accidentally created when first trying gerrit, made me think the change actually went in.

Anyway, I recreated the patch (fortunately small one) and pushed it through gerrit.  Please review when you have a sec.

http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f0a95edacea9363a1f96f816176d5bc6e6945668

Comment 9 Marc Khouzam 2012-05-01 13:42:18 EDT

(In reply to comment #8)
> Thanks Marc, I haven't had such a big miscommit with git in a while. 

I'm also still getting used to Gerrit :)

I put a couple of comments in Gerrit about the last patch.

Thanks

Comment 10 Marc Khouzam 2012-05-04 06:03:37 EDT

Re-opening so we don't forget to address the Gerrit review comments.  There is a bug in the Run menu where the 'reverse resume' does not appear.

Pawel, I can take care of it if you are busy.  However, I'm not sure how to deal with this:

If you toggle reverse debugging, then stop showing the debug view toolbar, the reverse buttons remain visible until I click on a debug context. Once I do, they disappear, except the toggle button. Then, if I click in another view, the toggle button disappears. The same thing happens when I re-enable the debug toolbar (buttons don't appear until I click around).

Exact steps are:
 - make debug view toolbar visible
 - enable reverse action set
 - start all-stop debug session with GDB >= 7.0
 - press toggle reverse button
 => all reverse button appear
 - hide the debug view toolbar
 => reverse buttons remain visible until clicking around

Comment 11 Pawel Piech 2012-05-04 14:35:45 EDT

Thanks Marc,  I'm sorry I hadn't noticed your gerrit comment previously.

I'm not sure what to do about updating the actions when "show debug toolbar" is toggled.  I can get it to behave correctly if I add: 
    exprService.requestEvaluation("org.eclipse.cdt.debug.ui.isReverseDebuggingEnabled"); //$NON-NLS-1$

to the launch action that handles the toggle and updates the system property.  Unfortunately the evaluation service does nothing for system properties.  So I could introduce a new property to wrap the system property, but it's essentially a new API, so it'll have to ask approvals to get it in.  

Any other ideas?

Comment 12 Pawel Piech 2012-05-04 18:20:31 EDT

Created attachment 215115 [details]
Patch for toggle toolbar visible.

This patch depends on the property being added in platform.  Will hold off with it until the dependency is clear.

Comment 13 Pawel Piech 2012-05-04 18:21:09 EDT

Created attachment 215116 [details]
The correct patch.

Comment 14 Marc Khouzam 2012-05-07 09:30:42 EDT

(In reply to comment #13)
> Created attachment 215116 [details]
> The correct patch.

Thank you Pawel!
Sorry I didn't reply on Friday.  If this solution can happen for Juno, it will be one less thing to keep track of, but it is not a show-stopper.  We had a poor refreshing of buttons for a while before, and people made due.  This case is even more rare, as it is only in the case a user changes the visibility of the debug toolbar, which will be very rare.

Comment 15 Marc Khouzam 2012-05-08 13:55:43 EDT

I pushed a tiny little fix to address the issue of not seeing the Reverse Resume button in the Run menu.

https://git.eclipse.org/r/5885

Pawel, can you tell me if you agree?

I suggest we then close this bug and open a different one for the issue of updating the actions when "show debug toolbar" is toggled.

Comment 16 Marc Khouzam 2012-05-08 13:57:37 EDT

About the fix I just pushed to Gerrit:

Before the fix, the 'reverse resume' operation is not visible in the run menu. I couldn't find a way to put it right above the 'resume' one without adding a groupMarker in the platform plugin. I think the best we can do is to use

locationURI="menu:org.eclipse.ui.run?after=stepGroup"

Which puts it a little lower, but I find it is better than putting it way at the top (using before=stepGroup).

This is what the fix does.

Comment 17 Pawel Piech 2012-05-08 14:23:05 EDT

(In reply to comment #15)
> I suggest we then close this bug and open a different one for the issue of
> updating the actions when "show debug toolbar" is toggled.

I agree.  I also +1'd on gerrit, so please go ahead and merge it in.

Thanks Marc,
Pawel

Comment 18 Marc Khouzam 2012-05-08 15:04:42 EDT

(In reply to comment #17)
> (In reply to comment #15)
> > I suggest we then close this bug and open a different one for the issue of
> > updating the actions when "show debug toolbar" is toggled.
> 
> I agree.  I also +1'd on gerrit, so please go ahead and merge it in.

Thanks.  I committed to master from Gerrit.

I've opened Bug 378892 about the remaining issue.