Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 331629

Summary: [disassembly] pageUp/pageDown scrolling jumps over unknown sections unexpectedly
Product: [Tools] CDT Reporter: Kirk Beitz <kirk.beitz>
Component: cdt-debug-dsfAssignee: Anton Leherbauer <aleherb+eclipse>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: normal    
Priority: P3 CC: aleherb+eclipse, cdtdoug, ken.ryall
Version: 7.0.1   
Target Milestone: 7.0.3   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
code to try to stay closer to previous focus when user scrolls via keypress
none
Alternative fix
aleherb+eclipse: iplog-
Alternative fix plus arrowUp/arrowDown cases aleherb+eclipse: iplog+, aleherb+eclipse: review+

Description Kirk Beitz CLA 2010-12-02 02:26:26 EST 
in the disassembly view, notice that upon performing pageUp a couple of times (and this will also occur with pageDown scrolling), a section of disassembly is displayed that is not adjacent to the disassembly that was being displayed to the user prior to the scroll.

it may then take a little effort to get back to the disassembly that the user was trying to look at in the first place.

i've noticed this for a while, and it has been a bit disconcerting, but i've tried to get used to it.

however, we are getting additional reports from folks who would prefer that pageUp/pageDown would always retrieve adjacent disassembly, as opposed to jumping over the gap, and then getting the disassembly on the other side of the gap.

in other words, when scrolling up, if a "..." section is encountered, check to see what operation caused that to occur, and if it was a pageUp, attempt to acquire the memory on the bottom of the gap.
Comment 1 Kirk Beitz CLA 2011-04-14 05:42:44 EDT 
Created attachment 193234 [details]
code to try to stay closer to previous focus when user scrolls via keypress

the attached patch attempts to improve the user experience by utilizing keypress scrolling to indicate a desire for continuity on the user's part (versus scrollbar scrolling, where the user may be more interested in scrolling over/past disabled ranges).

please review the patch and try it out.

for your own testing/trials … 

i've found it was most likely  to scroll over a disabled-range boundary (using the edc x86 debugger, so probably also with the gdb x86 debugger) if i set the cursor near the top of the disassembly window and then scrolled up or near the bottom and scrolled down, as this would be more likely to set the post-scroll cursor beyond the "..." location representing a disabled range.  after applying this patch, scrolling via pageUp/pageDown feels significantly improved to me, while still allowing using the scrollbar elevator or trackpad swipe-scrolling to jump over ranges as possibly desired.

(we would also like to apply this patch to the CDT_7_0 branch if it is considered ok to do so, since our next product update coming soon would still depend on that branch.)
Comment 2 Anton Leherbauer CLA 2011-04-14 08:59:11 EDT 
Created attachment 193251 [details]
Alternative fix

I tried your patch and it is an improvement, but I think it seems a bit fragile.  Twice I got into a situation where the view would no longer update.  This seems to be caused by the gotoAddress() call inside updateVisibleArea().
Anyway, I tried a different approach by directly handling the page up/down key presses.  Please give it a try.
Comment 3 Kirk Beitz CLA 2011-04-14 17:30:50 EDT 
Created attachment 193304 [details]
Alternative fix plus arrowUp/arrowDown cases

(In reply to comment #2)
> Created attachment 193251 [details]
> Alternative fix
> 
> I tried your patch and it is an improvement, but I think it seems a bit
> fragile.  Twice I got into a situation where the view would no longer update. 
> This seems to be caused by the gotoAddress() call inside updateVisibleArea().
> Anyway, I tried a different approach by directly handling the page up/down key
> presses.  Please give it a try.

thanks for your contribution!  page scrolling is much smoother with this approach, no doubt.

while i hadn't experienced complete the "no longer update" scenario you mentioned, scrolling was much jumpier with my original patch.

one thing i had been trying to address with my patch was arrow-up/arrow-down scrolling as well (as represented by the use of generic KEY in the original patch).

i've re-factored your patch a bit in my attempt to include arrowUp/arrowDown:

• combined pageUp() & pageDown() into keyScroll(int keyCode) in DisassemblyPart
  (the 2 algorithms already had much in common)

• return boolean indicating true if keyScroll() processes the key
  (this allows it to be ignored when no viewport change occurs)

my extended thanks in advance for your review of this new patch.
Comment 4 Anton Leherbauer CLA 2011-04-15 04:16:06 EDT 
Very good!  I like it.  Committed to HEAD and cdt_7_0.