-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support - New Recently Used Submenu #1837
base: master
Are you sure you want to change the base?
Support - New Recently Used Submenu #1837
Conversation
Introduced support for recently used submenus. Fixes:eclipse-platform#1530
@Dinesh0723 many thanks can you share some screenshots? |
Test Results 921 files ±0 921 suites ±0 1h 11m 31s ⏱️ - 4m 51s For more details on these errors, see this check. Results for commit b12c6ac. ± Comparison against base commit 66847ac. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Dinesh0723 for this contribution.
I just have some minor changes based on some coding practices, could you please incorporate them?
To be clear, I did not test the changes, my observations are merely about the code itself and not about the functionality.
Set<String> selectedFromOther = DynamicMenuSelection.getInstance().getSelectedFromOther(); | ||
selectedFromOther.addAll(dynamicMenuItem.getDynamicMenuItems()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this, this is a bad practice. If you want to add several items to that list then add a proper method in DynamicMenuSelection
and document it.
public Set<String> getSelectedFromOther() { | ||
return selectedFromOther; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return an immutable set to avoid calls like the one you used in WorkbenchPlugin.start()
. Adding stuff to this set should be done from within this class, otherwise it becomes really difficult to track who adds what.
import org.eclipse.core.runtime.preferences.IEclipsePreferences; | ||
import org.eclipse.core.runtime.preferences.InstanceScope; | ||
|
||
public class DynamicMenuItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some JavaDoc to the class and also to the public methods.
return new ArrayList<>(); | ||
} | ||
|
||
public void setDynamicMenuItems(Set<String> set) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this parameter to reflect what it represents and not what it is. One can see this is a set by looking at the type but one doesn't know what the set contains (I bet items
would be more suitable)
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/workbench/DynamicMenuItem.java
Show resolved
Hide resolved
/** | ||
* @since 3.132 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merged with already existing Javadoc block.
import org.eclipse.core.runtime.preferences.IEclipsePreferences; | ||
import org.eclipse.core.runtime.preferences.InstanceScope; | ||
|
||
public class DynamicMenuItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is to become a new API class, then it requires proper Javadoc and @since ...
markers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would be even better if this could remain internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, DynamicMenuItem is not a great name, especially if it's meant to serve only the newWizard case. I think it would be better to have this nested in the NewWizardNewPage or something, with more narrowly-scope names for preferences.
So it could be invoked with something like NewWizardNewPage.getItemsFromPreferences()
or something of that sort that reads more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickaelistria This cannot be nested in NewWizardNewPage
class as it is having default / Package visibility . Can I use NewWizard
class instead ? (Need to access from WorkbenchPlugin.java
)
IEclipsePreferences pref = InstanceScope.INSTANCE.getNode(PLUGIN_ID); | ||
String jsn = pref.get(DYNAMIC_MENU_ITEMS, null); | ||
if (jsn != null) { | ||
Gson gsn = new Gson(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GSon is a relatively heavy library to use for this case. Can you try avoiding it, for example by storing/reading the list as comma-separated ids and use pref.get(DYNAMIC_MENU_ITEM, "").split(",")
?
Changes requested in the PR eclipse-platform#1837 are addressed here
@mickaelistria , @fedejeanne Changes requested are addressed in PR #1922
@laeubi Shared here #1922 (comment) |
Changes requested in the PR eclipse-platform#1837 are addressed here
Introduced support for recently used submenus.
Fixes:#1530