Bug 202435 - [Navigation] "Previous change" button doesn't work properly
Summary: [Navigation] "Previous change" button doesn't work properly
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Compare (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 4.3   Edit
Assignee: Malgorzata Janczarska CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks: 383796 383797 406348
  Show dependency tree
 
Reported: 2007-09-06 09:08 EDT by Krzysztof Michalski CLA
Modified: 2013-04-23 12:35 EDT (History)
6 users (show)

See Also:
malgorzata.tomczyk: review+


Attachments
patch v0.1 (1.72 KB, patch)
2012-05-07 06:19 EDT, Marek Chodorowski CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Michalski CLA 2007-09-06 09:08:24 EDT
Build ID: I20070809-1105

Steps To Reproduce:
I have difference with some changes.
1. Clicking next change marks a proper part of text.     
2. Clicking previous change marks whole difference but should go to previous change and mark it.
Comment 1 Tomasz Zarna CLA 2007-09-17 08:12:14 EDT
Affirmative, it looks like clicking "Previous Change" does the same thing as "Previous Difference". The only difference is that it selects a part of text/file. I would mark it as a valid bug.
Comment 2 Tomasz Zarna CLA 2008-04-16 10:03:30 EDT
Let's make it a 'helpwanted' bug. I will also lower the priority to the value
appropriate for this kind of issues.
Comment 3 Marek Chodorowski CLA 2012-04-19 04:34:31 EDT
Hello. I would like to throw some light on this problem and clarify especially Krzysztof's 2nd step.

To illustrate how it currently works and how I believe it should work consider the 10 line file below where Cn indicates a change.

01
02 C1
03
04 C2
05 C3
06 C4
07
08 C5
09 C6
10

Starting at the top if you repeatedly click the NextChange icon the following will occur

NextChange -> Line 02 highlighted  (Difference 1)
NextChange -> change C1 highlighted
NextChange -> Lines 04, 05 and 06 highlighted (Difference 2)
NextChange -> change C2 highlighted
NextChange -> change C3 highlighted
NextChange -> change C4 highlighted
NextChange -> Lines 08 and 09 highlighted (Difference 3)
NextChange -> change C5 highlighted
NextChange -> change C6 highlighted
NextChange -> prompt to start from the top (if enabled)

Now if you start from the bottom and click PreviousChange you would expect the sequence to be reversed, that is the last difference should be highlighted, then each change in the diferences selected in reverse order then the previous difference, etc.  However, what you will see happen is:

PreviousChange -> Difference 3 highlighted
PreviousChange -> change C6 highlighted
PreviousChange -> Difference 2 highlighted
PreviousChange -> change C4 highlighted
PreviousChange -> Difference 1 highlighted
PreviousChange -> change C1 highlighted

My questions are:
1) Are you agree that this is right behavior?
2) Do you know if there are any plans to fix this bug in the near future?
Comment 4 Marek Chodorowski CLA 2012-05-07 06:19:40 EDT
Created attachment 215172 [details]
patch v0.1
Comment 5 Tomasz Zarna CLA 2012-05-07 06:32:27 EDT
(In reply to comment #4)
> Created attachment 215172 [details]
> patch v0.1

Thanks for the patch Marek! Could you please push it to Gerrit[1]. Also a unit test would be great. You can use SWTBot [2] if it's going to be a UI test, we can then add it to our unofficial (for the time being) UI suite[3].

[1] https://git.eclipse.org/r/#/
[2] http://www.eclipse.org/swtbot/
[3] https://github.com/zaza/org.eclipse.compare.tests.ui
Comment 6 Tomasz Zarna CLA 2012-06-28 09:45:16 EDT
Gosia, could you please have a look at the provided patch? Is it any good?
Comment 7 Malgorzata Janczarska CLA 2012-06-28 11:37:28 EDT
Pushed to Gerrit: https://git.eclipse.org/r/6541
Comment 8 Malgorzata Janczarska CLA 2012-07-02 10:14:54 EDT
I reviewed the patch and it's good. There was an error in calculating if the current change is first in the difference and the patch fixes it.  I think it's fine to merge it.
Comment 9 Malgorzata Janczarska CLA 2012-07-10 08:47:29 EDT
Fixed with 89abaa6f5d2de889ac809d81d969409130d2f0ba