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.

Sign up for GitHub

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

Jump to bottom

Add per-session recently used boards list #8607

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add per-session recently used boards list #8607

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0096b38
Refactor boardMenu creation
facchinm Mar 8, 2019
94f6bb5
Add per-session recently used boards list
facchinm Mar 6, 2019
9083508
Fix concurrent access to menuItemsToClickAfterStartup
facchinm Mar 7, 2019
b21cfa0
Make Recently used boards size configurable from preferences
facchinm Mar 8, 2019
c0710e0
Prepare for LRU persistency
facchinm Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 59 additions & 10 deletions app/src/processing/app/Base.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ public class Base {
Editor activeEditor;

private static JMenu boardMenu;
private static ButtonGroup boardsButtonGroup;
private static ButtonGroup recentBoardsButtonGroup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does recentBoardsButtonGroup need to be a ButtonGroup? AFAIU, a ButtonGroup is intended to group radio buttons so only one of them can be selected at once, but all recent-boards radiobuttons are also added to boardsButtonGroup, so they are already mutually exclusive. It seems you are using recentBoardsButtonGroup just to keep track of all recent-boards menu items, and then it is probably better to use a plain LinkedList or other collection?

private static Map<String, ButtonGroup> buttonGroupsMap;
private static List<JMenuItem> menuItemsToClickAfterStartup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this a static variable? It used to be a local variable, but now it is used in two places to pass to createBoardMenusAndCustomMenus. However, in the new place in rebuildRecentBoardsMenu, this list is not actually used after createBoardMenusAndCustomMenus fills it, so there is really no point in sharing the same list. It would be just as well to let rebuildRecentBoardsMenu have its own dummy list and pass that, or even pass null to indicate it does not actually care about this list (which requires some changes for createBoardMenusAndCustomMenus to support that).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this menuItemsToClickAfterStartup has a lot of code smell and is probably better removed entirely, but that seems out of scope here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, seems you partially fixed this in your next commit, where one of the occurences is again a local variable, but the other one now still uses the instance variable for no apparent reason. I would make both a local variable, and revert the name change with the leading _ in the next commit.

private static MenuScroller boardMenuScroller;

// these menus are shared so that the board and serial port selections
// are the same for all windows (since the board and serial port that are
Expand Down Expand Up @@ -1335,6 +1340,41 @@ public void selectTargetBoard(TargetBoard targetBoard) {
onBoardOrPortChange();
rebuildImportMenu(Editor.importMenu);
rebuildExamplesMenu(Editor.examplesMenu);
try {
rebuildRecentBoardsMenu();
} catch (Exception e) {
// fail silently
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? Please don't eat exceptions...

}
}

public void rebuildRecentBoardsMenu() throws Exception {

Enumeration<AbstractButton> btns = recentBoardsButtonGroup.getElements();
while (btns.hasMoreElements()) {
AbstractButton x = btns.nextElement();
if (x.isSelected()) {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this return?

}
}
btns = recentBoardsButtonGroup.getElements();
while (btns.hasMoreElements()) {
AbstractButton x = btns.nextElement();
boardMenu.remove(x);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why all this bookkeeping? It seems that rebuildRecentBoardsMenu() is only called from rebuildBoardsMenu(), which AFAICS starts with making boardsMenu() empty, so you can always just start from an empty slate here? Maybe that can be emphasized by renaming to addRecentBoardsMenuItems() or something like that, if you're worried about people calling this method from other contexts as well?

int index = 0;
for (TargetBoard board : BaseNoGui.getRecentlyUsedBoards()) {
JMenuItem item = createBoardMenusAndCustomMenus(boardsCustomMenus, menuItemsToClickAfterStartup,
buttonGroupsMap,
board, board.getContainerPlatform(), board.getContainerPlatform().getContainerPackage());
boardMenu.insert(item, 3);
item.setAccelerator(KeyStroke.getKeyStroke('0' + index,
Toolkit.getDefaultToolkit().getMenuShortcutKeyMask() |
ActionEvent.SHIFT_MASK));
recentBoardsButtonGroup.add(item);
boardsButtonGroup.add(item);
index ++;
}
boardMenuScroller.setTopFixedCount(3 + index);
}

public void onBoardOrPortChange() {
Expand Down Expand Up @@ -1432,7 +1472,8 @@ public void rebuildBoardsMenu() throws Exception {
// The first custom menu is the "Board" selection submenu
boardMenu = new JMenu(tr("Board"));
boardMenu.putClientProperty("removeOnWindowDeactivation", true);
MenuScroller.setScrollerFor(boardMenu).setTopFixedCount(1);
boardMenuScroller = MenuScroller.setScrollerFor(boardMenu);
boardMenuScroller.setTopFixedCount(1);

boardMenu.add(new JMenuItem(new AbstractAction(tr("Boards Manager...")) {
public void actionPerformed(ActionEvent actionevent) {
Expand Down Expand Up @@ -1472,21 +1513,26 @@ public void actionPerformed(ActionEvent actionevent) {
boardsCustomMenus.add(customMenu);
}

List<JMenuItem> menuItemsToClickAfterStartup = new LinkedList<>();
menuItemsToClickAfterStartup = new LinkedList<>();
boardsButtonGroup = new ButtonGroup();
recentBoardsButtonGroup = new ButtonGroup();
buttonGroupsMap = new HashMap<>();

ButtonGroup boardsButtonGroup = new ButtonGroup();
Map<String, ButtonGroup> buttonGroupsMap = new HashMap<>();
if (BaseNoGui.getRecentlyUsedBoards() != null) {
JMenuItem recentLabel = new JMenuItem(tr("Recently used boards"));
recentLabel.setEnabled(false);
boardMenu.add(recentLabel);
rebuildRecentBoardsMenu();
//rebuildRecentBoardsMenu(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commented line can be dropped?

}

// Cycle through all packages
boolean first = true;
for (TargetPackage targetPackage : BaseNoGui.packages.values()) {
// For every package cycle through all platform
for (TargetPlatform targetPlatform : targetPackage.platforms()) {

// Add a separator from the previous platform
if (!first)
boardMenu.add(new JSeparator());
first = false;
boardMenu.add(new JSeparator());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this separator be present for the first platform when there are no recently used boards? Rather than checking that, perhaps this could just see if boardMenu contains > 1 element and if so, add the separator?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems you fixed this in the next commit by always including the LRU header, I believe? I guess it will always have a value, since on startup some board will be selected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And a newer commit makes the LRU header again optional (when the preference is set to 0), so I again wonder if that produces a duplicate separator?

Copy link

Choose a reason for hiding this comment

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

Love to see that feature...
IMHO the last 4to5 recently used boards are fine, put persistent, not by session, please!
Thank you.


// Add a title for each platform
String platformLabel = targetPlatform.getPreferences().get("name");
Expand Down Expand Up @@ -1552,6 +1598,9 @@ public void actionPerformed(ActionEvent actionevent) {
for (final String menuId : customMenus.keySet()) {
String title = customMenus.get(menuId);
JMenu menu = getBoardCustomMenu(tr(title));
if (menu == null) {
continue;
}

if (board.hasMenu(menuId)) {
PreferencesMap boardCustomMenu = board.getMenuLabels(menuId);
Expand Down Expand Up @@ -1614,13 +1663,13 @@ private static boolean ifThereAreVisibleItemsOn(JMenu menu) {
return false;
}

private JMenu getBoardCustomMenu(String label) throws Exception {
private JMenu getBoardCustomMenu(String label) {
for (JMenu menu : boardsCustomMenus) {
if (label.equals(menu.getText())) {
return menu;
}
}
throw new Exception("Custom menu not found!");
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? Is this case suddenly a normal occurence with this LRU list? Or is this just an unrelated improvement (that would perhaps be better in a separate commit)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's indeed a separate commit, thanks

}

public List<JMenuItem> getProgrammerMenus() {
Expand Down
13 changes: 13 additions & 0 deletions arduino-core/src/processing/app/BaseNoGui.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,12 @@ static public void saveFile(String str, File file) throws IOException {
}
}

static private LinkedList<TargetBoard> recentlyUsedBoards = new LinkedList<TargetBoard>();

static public LinkedList<TargetBoard> getRecentlyUsedBoards() {
return recentlyUsedBoards;
}

static public void selectBoard(TargetBoard targetBoard) {
TargetPlatform targetPlatform = targetBoard.getContainerPlatform();
TargetPackage targetPackage = targetPlatform.getContainerPackage();
Expand All @@ -926,6 +932,13 @@ static public void selectBoard(TargetBoard targetBoard) {
File platformFolder = targetPlatform.getFolder();
PreferencesData.set("runtime.platform.path", platformFolder.getAbsolutePath());
PreferencesData.set("runtime.hardware.path", platformFolder.getParentFile().getAbsolutePath());

if (!recentlyUsedBoards.contains(targetBoard)) {
recentlyUsedBoards.add(targetBoard);
}
if (recentlyUsedBoards.size() > 4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the size of the LRU list be a preference? I'm pretty sure people will be asking for more entries soon :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, a preference will not hurt and will be future proof 🙂

recentlyUsedBoards.remove();
}
}

public static void selectSerialPort(String port) {
Expand Down