Bug 427887 - [CommonNavigator] [working sets] Allow working set in Project Navigator to be sorted manually
Summary: [CommonNavigator] [working sets] Allow working set in Project Navigator to be...
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.4   Edit
Hardware: PC Linux
: P3 enhancement with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL: https://git.eclipse.org/r/24951
Whiteboard:
Keywords: noteworthy
: 575357 (view as bug list)
Depends on: 452632
Blocks: 427897
  Show dependency tree
 
Reported: 2014-02-11 07:20 EST by Mickael Istria CLA
Modified: 2021-08-12 04:37 EDT (History)
6 users (show)

See Also:
mistria: review?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mickael Istria CLA 2014-02-11 07:20:56 EST
Similarly to what's available in JDT package explorer, it would be convenient to be able to sort working sets in the proejct explorer.
The working set implementation of Package Explorer could be ported to Project Explorer.
Comment 1 Dani Megert CLA 2014-02-11 07:45:53 EST

*** This bug has been marked as a duplicate of bug 271230 ***
Comment 2 Mickael Istria CLA 2014-02-11 07:49:43 EST
This does not seem to be a DUP of bug 271230 . Indeed bug 271230 is about order of projects inside working set, whereas this request is about the order of working sets one related to the other.

Related discussion on mailing-list: https://dev.eclipse.org/mhonarc/lists/jdt-dev/msg00736.html
Comment 3 Mickael Istria CLA 2014-04-11 08:51:25 EDT
@Timo: you're the one who mentioned it on the jdt-dev mailing-list. I had a quick look and couldn't find a way to order working sets one with the other from the "Package Explorer" view.
Did I understand correctly your message? If so, can you please details how you can achieve that?
Comment 4 Sebastian Zarnekow CLA 2014-04-11 08:53:11 EDT
You have to choose 'Configure working sets' and than use 'up' and 'down' to get the order you want.
Comment 5 Mickael Istria CLA 2014-04-11 08:56:38 EDT
Ok, I see. It only shows up on Package Explorer when top-level elements are working sets.
Comment 6 Mickael Istria CLA 2014-04-14 06:41:43 EDT
Suggested patch: https://git.eclipse.org/r/24951
The UI is accessible from the "Select Working Sets..." menu of Package Explorer.
This contain an API change (addition of a method on public interface). This implementation also change default behaviour so that working sets are by default listed in order of creation, and not in alphabetic order.
Comment 7 Mickael Istria CLA 2014-07-15 04:41:33 EDT
Since there is a review pending, couldn't this review be planned for 4.5? The issue with letting contribution pending is that they quickly become obsolete and it's not really motivating for contributors to keep on contributing if patches get ignored.
Comment 8 Lars Vogel CLA 2014-11-17 07:36:48 EST
(In reply to Mickael Istria from comment #0)
> Similarly to what's available in JDT package explorer, it would be
> convenient to be able to sort working sets in the proejct explorer.

Of topic, but how can I sort working sets in the JDT package explorer?
Comment 9 Mickael Istria CLA 2014-11-17 07:40:16 EST
(In reply to Lars Vogel from comment #8)
> Of topic, but how can I sort working sets in the JDT package explorer?

I find it fully inside the topic ;) JDT Package Explorer provides 2 ways AFAIK:
* In the view menu of Package Explorer, Configure Working Sets, you see some up/down buttons that allow to sort working sets
* By DND in the navigation tree: dropping a working sets just before or after another.

My current patch supports the 1st approach. If it gets accepted, I'll provide a patch for the 2nd one.
Comment 10 Daniel Rolka CLA 2014-11-20 05:38:44 EST
Released to master as: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4a758782d855203c5181dbd12dfca229cb2d0346

Thanks for patch Mickael,

Daniel
Comment 11 Mickael Istria CLA 2014-11-20 05:39:20 EST
Thanks for the review!
Comment 12 Lars Vogel CLA 2014-11-20 06:47:12 EST
Mickael, great thanks a bunch. Can you provide a entry for the N&N for M4?
Comment 13 Daniel Rolka CLA 2014-11-20 08:34:36 EST
(In reply to Lars Vogel from comment #12)
> Mickael, great thanks a bunch. Can you provide a entry for the N&N for M4?

Yes, the N&N entry would be great

Daniel
Comment 14 Daniel Rolka CLA 2014-11-20 08:36:32 EST
I've pushed to master a small polishing of API, released as:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=878cb7c3a0088714b33c7e59db55ea9ce05dd5b2

Daniel
Comment 15 Mickael Istria CLA 2014-11-20 08:58:14 EST
Suggested N&N entry: https://git.eclipse.org/r/36763
Comment 16 Dani Megert CLA 2014-11-21 08:41:55 EST
(In reply to Daniel Rolka from comment #14)
> I've pushed to master a small polishing of API, released as:
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=878cb7c3a0088714b33c7e59db55ea9ce05dd5b2

This causes Javadoc compile warnings in the workspace and in our official build. Didn't you see them in your workspace?

Fixed with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2f5e22a4514420ca28923f9e4b6e2a3c6df1b734
Comment 17 Daniel Rolka CLA 2014-11-26 08:35:12 EST
As I wrote in the Bug 453181 the change modifies the API behavior that can corrupt clients that use it. It will be safer to add the new API that returns working sets as array sorted by adding order or manual reordering. I will revert the change

Daniel
Comment 19 Daniel Rolka CLA 2014-11-26 08:40:35 EST
Mickael,

Please update the patch with the proper new API for the IWorkingSetManager class and send to Gerrit to review (please assign me as reviewer for it)

The feature is interesting for me so I would like to push it in M4

Daniel
Comment 20 Daniel Rolka CLA 2014-11-26 08:49:21 EST
(In reply to Daniel Rolka from comment #17)
> As I wrote in the Bug 453181 the change modifies the API behavior that can
> corrupt clients that use it. It will be safer to add the new API that
> returns working sets as array sorted by adding order or manual reordering. I
> will revert the change
> 
> Daniel

I think we can add the new ArrayList for the AbstractWorkingSetManager class that will keep working sets sorted by adding order/manual reordering (as original patch does), expose it by the new API and use it in the updated version of the patch

Daniel
Comment 21 Mickael Istria CLA 2014-11-26 09:08:05 EST
Which exact API contract was broken by my previous change? Looking at the code, it doesn't seem to me that explicits how working sets are expected to be sorted.
About the test that checks that Working Sets were sorted by name, then it seems to me that it's not an API test, and that it would be better to remove the assertion in the test which is testing more that what's the API is providing.
Comment 22 Daniel Rolka CLA 2014-11-26 09:24:59 EST
(In reply to Mickael Istria from comment #21)
> Which exact API contract was broken by my previous change? Looking at the
> code, it doesn't seem to me that explicits how working sets are expected to
> be sorted.
> About the test that checks that Working Sets were sorted by name, then it
> seems to me that it's not an API test, and that it would be better to remove
> the assertion in the test which is testing more that what's the API is
> providing.

The IWorkingSetManager.getWorkingSets() returns the array sorted by the WorkingSetComparator. It is possible that someone bases on it. The additional array for working sets seems to be simple solution and will allow to keep the old API unmodified.

Daniel
Comment 23 Daniel Rolka CLA 2014-11-26 09:26:24 EST
(In reply to Daniel Rolka from comment #22)

> The additional array

The additional ArrayList

Daniel
Comment 24 Mickael Istria CLA 2014-11-26 09:28:41 EST
Well, I'll try to make another shot to this patch when I have time. But I'm still not convinced that Eclipse Platform has interest in guaranteeing such behaviours if they're not specified by API. The WorkingSetComparator is internat and is an implementation choice, so anyone who relies on internal behaviour and implementation choices instead of API specification and contracts is aware that they're taking the risk of some behavioural change.
TLDR; http://xkcd.com/1172/
Comment 25 Daniel Rolka CLA 2014-11-26 09:45:04 EST
Dani,

Could you please advice here?

Daniel
Comment 26 Dani Megert CLA 2014-11-26 11:42:33 EST
(In reply to Daniel Rolka from comment #25)
> Dani,
> 
> Could you please advice here?
> 
> Daniel

The bug is that the sorting wasn't mentioned in the Javadoc (Daniel, please fix this). There are 3 rational reasons why the current behavior is the desired one:

1. No one sorts in the code/implementation if not desired/needed.
2. No one adds a test to verify 1 if not really intended to work like this.
3. The current clients don't sort, but in the UI it must be sorted to be usable.

For all those reasons we can't change the current API.
Comment 27 Mickael Istria CLA 2014-11-26 12:25:23 EST
@Daniel R: before doing anything, I'd like to make sure I understand what you're suggesting/expecting.
* Add List manuallySortedWorkingSets to AbstractWorkingSetManager, and keep the workingSets SortedSet (that we could rename workingSetsByName)
* Add to the add/remove methods also addition/removal to the list (as well as the set)
* Use the manuallySortedWorkingSets as the structure to use for persistence
* Do not touch the getWorkingSets method
* Create a getManuallySortedWorkingSets method based on the List (that could even return an ImmutableList instead of an Array)
?

Overall, that's possible, but I really have the feeling that it shouldn't be the responsibility of Platform UI code to deal with issue that bad clients have (ie assuming things about order of working sets when there is nothing recommending to assume that).
Comment 28 Dani Megert CLA 2014-11-26 12:32:10 EST
(In reply to Mickael Istria from comment #27)
> Overall, that's possible, but I really have the feeling that it shouldn't be
> the responsibility of Platform UI code to deal with issue that bad clients
> have (ie assuming things about order of working sets when there is nothing
> recommending to assume that).

The only bad thing was that it was missed to document in the Javadoc by the developer. Like code, Javadoc can have bugs.
Comment 29 Dani Megert CLA 2014-11-27 05:03:48 EST
We must not add the 'Up'/'Down' buttons to the 'Select Working Set' dialog if the view shows the projects as top-level elements. In that case the user always wants to see the working sets in sorted order, so that he can easily choose. In addition, the order would not be honored by the explorer anyway, and that's good so.

I also think the feature to quickly switch back to sorted again is important if you have many working sets. Otherwise you have to manually sort them via 'Up'/'Down'.

Maybe we could simply push down JDT's 'ConfigureWorkingSetAction' and 'WorkingSetConfigurationDialog'?
Comment 30 Mickael Istria CLA 2014-11-27 05:11:14 EST
I guess moving and reusing the JDT dialogs would be ideal given the goal of this bug (ie make Project Explorer have all good features from JDT Package Explorer).
Comment 31 Dani Megert CLA 2014-11-27 05:15:09 EST
(In reply to Mickael Istria from comment #30)
> I guess moving and reusing the JDT dialogs would be ideal given the goal of
> this bug (ie make Project Explorer have all good features from JDT Package
> Explorer).

OK. Can you create a Gerrit change for that? I'm willing to review when ready.
Comment 32 Mickael Istria CLA 2014-11-27 05:17:40 EST
(In reply to Dani Megert from comment #31)
> (In reply to Mickael Istria from comment #30)
> > I guess moving and reusing the JDT dialogs would be ideal given the goal of
> > this bug (ie make Project Explorer have all good features from JDT Package
> > Explorer).
> 
> OK. Can you create a Gerrit change for that? I'm willing to review when
> ready.

I'll do it when it gets to the top of my todo-list, but my todo-list contains several higher priority stuff at the moment, so I can't guarantee I'm able to write a patch it time for review and inclusion in M4.
Comment 33 Dani Megert CLA 2014-11-27 05:21:15 EST
(In reply to Mickael Istria from comment #32)
> (In reply to Dani Megert from comment #31)
> > (In reply to Mickael Istria from comment #30)
> > > I guess moving and reusing the JDT dialogs would be ideal given the goal of
> > > this bug (ie make Project Explorer have all good features from JDT Package
> > > Explorer).
> > 
> > OK. Can you create a Gerrit change for that? I'm willing to review when
> > ready.
> 
> I'll do it when it gets to the top of my todo-list, but my todo-list
> contains several higher priority stuff at the moment, so I can't guarantee
> I'm able to write a patch it time for review and inclusion in M4.

Sure, np. It's the same on my side.
Comment 34 Dani Megert CLA 2014-12-03 08:57:24 EST
*** Bug 453983 has been marked as a duplicate of this bug. ***
Comment 35 Lars Vogel CLA 2015-01-22 14:30:03 EST
(In reply to Mickael Istria from comment #32) 
> I'll do it when it gets to the top of my todo-list, but my todo-list
> contains several higher priority stuff at the moment, so I can't guarantee
> I'm able to write a patch it time for review and inclusion in M4.

I remove the planning for Eclipse 4.5, based on this comment. Once we have again a patch, we can retarget.
Comment 36 Mickael Istria CLA 2015-06-10 11:34:38 EDT
I just had a look at moving WorkingSetConfigurationDialog and there is a huge WorkingSetModel class in JDT that should be moved together with it.
I'm not familiar with this part of JDT. What's in JDT working set mechanism that isn't provided by Platform UI? What do we really need to move to provide same amount of functionality?

It seems to me that trying to migrate JDT parts is actually more difficult than just providing the feature in Project Explorer.

Another question is where we would like the working set sorting to be stored? In the view memento or in the whole IDE (in the WorkingSetManager) ?
Comment 37 Dani Megert CLA 2015-06-11 11:42:26 EDT
(In reply to Mickael Istria from comment #36)
> I just had a look at moving WorkingSetConfigurationDialog and there is a
> huge WorkingSetModel class in JDT that should be moved together with it.
> I'm not familiar with this part of JDT. What's in JDT working set mechanism
> that isn't provided by Platform UI? What do we really need to move to
> provide same amount of functionality?

Manual sorting ;-). Others working set, store and restore state.


> Another question is where we would like the working set sorting to be
> stored? In the view memento or in the whole IDE (in the WorkingSetManager) ?

The view (done by WorkingSetModel). You can have a Project Explorer each windows but sorted differently.
Comment 38 Mickael Istria CLA 2021-08-12 04:37:04 EDT
*** Bug 575357 has been marked as a duplicate of this bug. ***