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 all commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
142 changes: 121 additions & 21 deletions app/src/processing/app/Base.java
View file
Open in desktop
Expand Up @@ -117,6 +117,13 @@ public class Base {
List<Editor> editors = Collections.synchronizedList(new ArrayList<Editor>());
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
// actually used are determined by the preferences, which are shared)
Expand Down Expand Up @@ -552,6 +559,36 @@ protected boolean restoreSketches() throws Exception {
return (opened > 0);
}

protected boolean restoreRecentlyUsedBoards() throws Exception {
// Iterate through all sketches that were open last time p5 was running.
// If !windowPositionValid, then ignore the coordinates found for each.

// Save the sketch path and window placement for each open sketch
int count = PreferencesData.getInteger("last.recent_boards.count");
int opened = 0;
for (int i = count - 1; i >= 0; i--) {
String fqbn = PreferencesData.get("last.recent_board" + i + ".fqbn");
if (fqbn == null) {
continue;
}
//selectTargetBoard(new TargetBoard());
}
return count != 0;
}

/**
* Store list of sketches that are currently open.
* Called when the application is quitting and documents are still open.
*/
protected void storeRecentlyUsedBoards() {
int i = 0;
for (TargetBoard board : BaseNoGui.getRecentlyUsedBoards()) {
PreferencesData.set("last.recent_board" + i + ".fqbn", board.getFQBN());
i++;
}
PreferencesData.setInteger("last.recent_boards.count", BaseNoGui.getRecentlyUsedBoards().size());
}

/**
* Store screen dimensions on last close
*/
Expand Down Expand Up @@ -1313,6 +1350,63 @@ public void rebuildExamplesMenu(JMenu menu) {
private static String priorPlatformFolder;
private static boolean newLibraryImported;

public void selectTargetBoard(TargetBoard targetBoard) {
for (int i = 0; i < boardMenu.getItemCount(); i++) {
JMenuItem menuItem = boardMenu.getItem(i);
if (!(menuItem instanceof JRadioButtonMenuItem)) {
continue;
}

JRadioButtonMenuItem radioButtonMenuItem = ((JRadioButtonMenuItem) menuItem);
if (targetBoard.getName().equals(radioButtonMenuItem.getText())) {
radioButtonMenuItem.setSelected(true);
break;
}
}

BaseNoGui.selectBoard(targetBoard);
filterVisibilityOfSubsequentBoardMenus(boardsCustomMenus, targetBoard, 1);

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('1' + index,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems an unrelated off-by-one bugfix that should have been squashed into the previous 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.

Not an off by one really but a different shortcut, so I'd leave it as a separate commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean that this shortcut is introduced in the previous commit and changed in this commit, so there is not really a point in separating them? You might as well introduce the shortcut correctly in the first commit? Also, this change (short 0 to 1) is currently in the " Fix concurrent access to menuItemsToClickAfterStartup" commit, where it does not belong at all AFAICS? So it should at least be moved into its own commit then.

Toolkit.getDefaultToolkit().getMenuShortcutKeyMask() |
ActionEvent.SHIFT_MASK));
recentBoardsButtonGroup.add(item);
boardsButtonGroup.add(item);
index ++;
}
boardMenuScroller.setTopFixedCount(3 + index);
}

public void onBoardOrPortChange() {
BaseNoGui.onBoardOrPortChange();

Expand Down Expand Up @@ -1406,9 +1500,10 @@ public void rebuildBoardsMenu() throws Exception {
boardsCustomMenus = new LinkedList<>();

// The first custom menu is the "Board" selection submenu
JMenu boardMenu = new JMenu(tr("Board"));
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 @@ -1448,21 +1543,26 @@ public void actionPerformed(ActionEvent actionevent) {
boardsCustomMenus.add(customMenu);
}

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

boolean hasRecentBoardsMenu = (PreferencesData.getInteger("editor.recent_boards.size", 4) != 0);

ButtonGroup boardsButtonGroup = new ButtonGroup();
Map<String, ButtonGroup> buttonGroupsMap = new HashMap<>();
if (hasRecentBoardsMenu) {
JMenuItem recentLabel = new JMenuItem(tr("Recently used boards"));
recentLabel.setEnabled(false);
boardMenu.add(recentLabel);
}

// 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 All @@ -1476,7 +1576,7 @@ public void actionPerformed(ActionEvent actionevent) {
for (TargetBoard board : targetPlatform.getBoards().values()) {
if (board.getPreferences().get("hide") != null)
continue;
JMenuItem item = createBoardMenusAndCustomMenus(boardsCustomMenus, menuItemsToClickAfterStartup,
JMenuItem item = createBoardMenusAndCustomMenus(boardsCustomMenus, _menuItemsToClickAfterStartup,
buttonGroupsMap,
board, targetPlatform, targetPackage);
boardMenu.add(item);
Expand All @@ -1485,14 +1585,16 @@ public void actionPerformed(ActionEvent actionevent) {
}
}

if (menuItemsToClickAfterStartup.isEmpty()) {
menuItemsToClickAfterStartup.add(selectFirstEnabledMenuItem(boardMenu));
if (_menuItemsToClickAfterStartup.isEmpty()) {
_menuItemsToClickAfterStartup.add(selectFirstEnabledMenuItem(boardMenu));
}

for (JMenuItem menuItemToClick : menuItemsToClickAfterStartup) {
for (JMenuItem menuItemToClick : _menuItemsToClickAfterStartup) {
menuItemToClick.setSelected(true);
menuItemToClick.getAction().actionPerformed(new ActionEvent(this, -1, ""));
}

menuItemsToClickAfterStartup = _menuItemsToClickAfterStartup;
}

private JRadioButtonMenuItem createBoardMenusAndCustomMenus(
Expand All @@ -1512,12 +1614,7 @@ private JRadioButtonMenuItem createBoardMenusAndCustomMenus(
@SuppressWarnings("serial")
Action action = new AbstractAction(board.getName()) {
public void actionPerformed(ActionEvent actionevent) {
BaseNoGui.selectBoard((TargetBoard) getValue("b"));
filterVisibilityOfSubsequentBoardMenus(boardsCustomMenus, (TargetBoard) getValue("b"), 1);

onBoardOrPortChange();
rebuildImportMenu(Editor.importMenu);
rebuildExamplesMenu(Editor.examplesMenu);
selectTargetBoard((TargetBoard) getValue("b"));
}
};
action.putValue("b", board);
Expand All @@ -1533,6 +1630,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 @@ -1595,13 +1695,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
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() > PreferencesData.getInteger("editor.recent_boards.size", 4)) {
recentlyUsedBoards.remove();
}
}

public static void selectSerialPort(String port) {
Expand Down