Bug 564941 - Drillstack#goHome() should call Deque#getLast() not Deque#getFirst()
Summary: Drillstack#goHome() should call Deque#getLast() not Deque#getFirst()
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.15   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.17 M3   Edit
Assignee: Phil Beauvoir CLA
QA Contact:
URL:
Whiteboard:
Keywords: regression
Depends on: 551135
Blocks:
  Show dependency tree
 
Reported: 2020-07-05 13:09 EDT by Phil Beauvoir CLA
Modified: 2020-07-20 08:29 EDT (History)
2 users (show)

See Also:


Attachments
Snippet that shows the problem (3.44 KB, text/plain)
2020-07-06 06:02 EDT, Phil Beauvoir CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Beauvoir CLA 2020-07-05 13:09:36 EDT
This bug has been opened as a continuation of Bug #551135

Drillstack#goHome() should get the last element in the Deque, not the fisrt

Currently:

public DrillFrame goHome() {
   DrillFrame aFrame = fStack.getFirst();
   reset();
   return aFrame;
}

Should be this:

public DrillFrame goHome() {
   DrillFrame aFrame = fStack.getLast();
   reset();
   return aFrame;
}
Comment 1 Eclipse Genie CLA 2020-07-05 14:11:56 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/165853
Comment 2 Lars Vogel CLA 2020-07-06 02:37:20 EDT
Thanks, Phil. How can I test your fix?
Comment 3 Phil Beauvoir CLA 2020-07-06 03:57:00 EDT
(In reply to Lars Vogel from comment #2)
> Thanks, Phil. How can I test your fix?

Lars, are there unit tests for the Drillstack class to test it when it was changed to use an ArrayDegue rather than a Stack class? Or does Eclipse or one of its plug-ins use the DrillDownAdapter with "Home"?

Otherwise, we can see from the documentation and source code of the Degue Interface and ArrayDeque class that it pushes elements in the following order (so that the most recent node pushed is at position zero):

[0] Node 2
[1] Node 1
[2] Home Node

So we have to get the last one when we call goHome().

(This is the opposite to the Stack class)
Comment 4 Phil Beauvoir CLA 2020-07-06 04:20:21 EDT
See the comments here about reverse iteration orders:

https://stackoverflow.com/questions/12524826/why-should-i-use-deque-over-stack
Comment 5 Phil Beauvoir CLA 2020-07-06 06:02:32 EDT
Created attachment 283520 [details]
Snippet that shows the problem

Lars, attached is a snippet that shows the problem. Try it before and after my patch.
Comment 6 Lars Vogel CLA 2020-07-06 07:43:36 EDT
Thanks Phil, I check later this week and if all is good, I merge it early next week (this week is M1 freeze week).
Comment 7 Karsten Thoms CLA 2020-07-13 03:01:36 EDT
Phil, could you try to translate your snippet into a test case? I think it is pretty close to one. For Drillstack there is no test case yet. As this is closely related to TreeViewer org.eclipse.jface.tests might be the appropriate place, and org.eclipse.jface.tests.viewers.interactive looks as an appropriate package.
Comment 8 Phil Beauvoir CLA 2020-07-13 03:04:26 EDT
Karsten,

I'm not sure if I can do that. I already had big problems getting the Eclipse code to compile when I got it from Gerrit. So many dependencies would not compile.
Comment 9 Karsten Thoms CLA 2020-07-13 03:32:01 EDT
OK, let me see what I can do. I'll try to find time to add a test to your change.

FYI: Setting up a working dev env is quite easy using Oomph: https://wiki.eclipse.org/Platform_UI/How_to_Contribute/Oomph
For "Product" I use Eclipse IDE for Eclipse Committers"
For "Projects" I typically choose "JDT, Equinox, Platform, PDE".
Comment 11 Karsten Thoms CLA 2020-07-13 18:17:45 EDT
Thanks Phil for your contribution! The snippet was quite helpful, I've created the test case from it.
Comment 12 Phil Beauvoir CLA 2020-07-14 03:28:06 EDT
Thanks for that, Karsten.
Comment 13 Lars Vogel CLA 2020-07-20 08:29:42 EDT
(In reply to Phil Beauvoir from comment #12)
> Thanks for that, Karsten.

Thanks to Phil and Karsten for this fix.