Skip to content
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

Show recently used "File > New" wizards #1922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lathapatil
Copy link
Contributor

@lathapatil lathapatil commented Jun 3, 2024

Changes requested in the PR
#1837 are addressed here

Fixes : #1530

@iloveeclipse
Copy link
Member

@lathapatil : could you please provide some screenshot showing the change?

Copy link
Contributor

github-actions bot commented Jun 3, 2024

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 51m 16s ⏱️ -6s
 7 712 tests ±0   7 484 ✅ +4  228 💤 ±0  0 ❌  - 3 
24 297 runs  ±0  23 550 ✅ +4  747 💤 ±0  0 ❌  - 3 

Results for commit 3683ab5. ± Comparison against base commit 3b49502.

♻️ This comment has been updated with latest results.

@lathapatil
Copy link
Contributor Author

lathapatil commented Jun 3, 2024

@iloveeclipse Yes, PFB for recently used 5 new shortcuts

recentlyUsedNewWizards

@lathapatil lathapatil changed the title Show recently used File > New wizards Show recently used "File > New" wizards Jun 4, 2024
@iloveeclipse iloveeclipse force-pushed the Enhancements/1530_Recent_File_New_Submenus branch from 295f697 to 9f5a9d4 Compare July 9, 2024 15:32
@iloveeclipse iloveeclipse force-pushed the Enhancements/1530_Recent_File_New_Submenus branch from 9f5a9d4 to aecf9a8 Compare October 2, 2024 06:18
@lathapatil lathapatil force-pushed the Enhancements/1530_Recent_File_New_Submenus branch from aecf9a8 to 92cba6d Compare October 21, 2024 11:02
@@ -186,9 +191,28 @@ protected void addItems(List<IContributionItem> list) {
list.add(new ActionContributionItem(newExampleAction));
list.add(new Separator());
}
// To add shortcuts from OTHER... wizard regardless of perspective
Collection<IContributionItem> otherItems = new LinkedList<>();
if (!RecentNewWizardSelection.getInstance().getSelectedFromOther().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name getSelectedFromOther() and derived variable names are unfortunate, I had no idea how to understand these names.
Why not just RecentNewWizardSelection.getInstance().getItems() ?

* Returns the action for the given wizard id, or null if not found.
*
* @since 3.132
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please make the method package protected, as we don't expect anyone to overwrite it.
  2. Please remove "since" comment after making the method package protected, as it would not be new API anymore.
  3. If this would be changed to "public" as proposed, this would be wrong version, API tooling shows an error in the IDE. It should be 3.134 and MANIFESt.MF version should be changed from 3.133.100 to 3.134.0, because new API method would be added.

@laeubi, @HannesWell : looks like API tooling validation on jenkins stopped to work (second time I see it after #2430).
Could you please check whether is that tycho, PDE or jenkins/maven configuration issue?

@@ -210,6 +217,7 @@ public class WorkbenchPlugin extends AbstractUIPlugin {
public WorkbenchPlugin() {
super();
inst = this;
recentNewWizardsPreferenceManager = new RecentNewWizardsPreferenceManager();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never ever do anything in bundle constructor! Any bundle initialization code should be added in the start() method.

@@ -766,6 +774,10 @@ public void start(BundleContext context) throws Exception {
// to be loaded.s
if (uiBundle != null)
uiBundle.start(Bundle.START_TRANSIENT);

List<String> recentlyUsedNewWizards = recentNewWizardsPreferenceManager.getMenuShortcutsFromPreferences();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code doesn't belong to the bundle activator, it is an internal implementation detail of the RecentNewWizardsPreferenceManager and should be done there.

@@ -1048,6 +1060,9 @@ public void stop(BundleContext context) throws Exception {
testableTracker.close();
testableTracker = null;
}
// Store recently used new page shortcuts to preferences
Set<String> selectedFromOther = RecentNewWizardSelection.getInstance().getSelectedFromOther();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code doesn't belong to the bundle activator, it is an internal implementation detail of the RecentNewWizardsPreferenceManager and should be done there. What yuo could do is to call RecentNewWizardsPreferenceManager.shutdown() here.

@@ -689,4 +689,11 @@ public IWorkbenchWizard createWizard() throws CoreException {

updateDescription(selectedObject);
}

/**
* @return Returns the selectedElement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is meaningless. Instead, one could provide information that the selection could be null

@@ -94,6 +94,7 @@ public void createControl(Composite parent) {
* they will persist into the next invocation of this wizard page
*/
protected void saveWidgetValues() {
RecentNewWizardSelection.getInstance().addItem(newResourcePage.getSelectedElement().getId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newResourcePage.getSelectedElement() may return null, so please add check before.

* of the menu manager with New Wizard actions. The recently used new wizards
* will appear before the "Other" shortcut.
*
* @since 3.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove "since", this package is not API

public class RecentNewWizardSelection {
private Set<String> selectedFromOther = new LinkedHashSet<>();
private static RecentNewWizardSelection instance;
private static final int MAX_MENU_SIZE = 5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you intended to have 5 recent items: currently only four are kept.

Copy link
Contributor Author

@lathapatil lathapatil Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you intended to have 5 recent items: currently only four are kept.

Could you please share the scenario you tested .
It do not add the shortcuts if it is already present as part of fixed shortcuts (highlighted in the plugin perspective image) of current perspective . The last used shortcuts change if you switch the perspective .

Java perspective

image

When I switch to plugin perspective ..

image

* @return the set of recently used new wizard menu shortcut IDs
*/

public Set<String> getSelectedFromOther() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to rename getSelectedFromOther to getItems() or getMenuIds() or getIds()

iloveeclipse added a commit to iloveeclipse/eclipse.platform.ui that referenced this pull request Oct 22, 2024
Check if API tools are working in platform UI.

This PR should fail validation because it adds new API with wrong since
tag.

See eclipse-platform#2430
See eclipse-platform#1922
@iloveeclipse iloveeclipse mentioned this pull request Oct 22, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform.ui that referenced this pull request Oct 22, 2024
Check if API tools are working in platform UI.

This PR should fail validation because it adds new API with wrong since
tag.

See eclipse-platform#2430
See eclipse-platform#1922
Changes requested in the PR
eclipse-platform#1837 are
addressed here
Review points addressed
@lathapatil lathapatil force-pushed the Enhancements/1530_Recent_File_New_Submenus branch from 92cba6d to 3683ab5 Compare October 25, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support New > Recently Used
2 participants