Add per-session recently used boards list #8607

facchinm · 2019-03-06T17:51:31Z

The list appears at the top of Board submenu
Boards are also reachable via a CTRL+SHIFT+INDEX

This doesn't really fix, but is equivalent to arduino/arduino-ide#2339

ArduinoBot · 2019-03-07T13:56:02Z

✅ Build completed.

Please test this code using one of the following:

⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-831-linux32.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-831-linux64.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-831-windows.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-831-macosx.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-831-linuxarm.tar.xz

ℹ️ The linuxarm build is still experimental and may not be always available.

matthijskooijman

Good to see this change, will be very helpful. However, I wonder if we should persist the LRU list right away. Only keeping one per session is helpful if you're working with two boards at the same time, but typically I'll work with one board on one day, then with another board another day (and now I end up scrolling to a long list). If the LRU list is persisted, it will gradually become a list of boards you have / regularly use. For that to work, it might need a bit more than 4 entries, though.

Finally, I was a bit confused with your commit message saying "CTRL+SHIFT+INDEX", thinking "Where is the INDEX key?". Not a big deal, but perhaps rephrase that as "CTRL+SHIFT+" or something?

matthijskooijman · 2019-03-07T13:47:03Z

app/src/processing/app/Base.java

-        if (!first)
-          boardMenu.add(new JSeparator());
-        first = false;
+        boardMenu.add(new JSeparator());


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?

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.

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?

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

matthijskooijman · 2019-03-07T13:58:38Z

app/src/processing/app/Base.java

+  private static ButtonGroup boardsButtonGroup;
+  private static ButtonGroup recentBoardsButtonGroup;
+  private static Map<String, ButtonGroup> buttonGroupsMap;
+  private static List<JMenuItem> menuItemsToClickAfterStartup;


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).

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

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.

matthijskooijman · 2019-03-07T14:01:05Z

app/src/processing/app/Base.java

+      rebuildRecentBoardsMenu();
+    } catch (Exception e) {
+      // TODO Auto-generated catch block
+      e.printStackTrace();


Why is this try block needed? I suppose that GUI modifications can throw, but why doesn't the same hold for e.g. rebuildExamplesMenu()? Does that catch exceptions internally? Should rebuildRecentBoardsMenu() do the same?

matthijskooijman · 2019-03-07T14:02:17Z

app/src/processing/app/Base.java

    for (JMenu menu : boardsCustomMenus) {
      if (label.equals(menu.getText())) {
        return menu;
      }
    }
-    throw new Exception("Custom menu not found!");
+    return null;


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)?

It's indeed a separate commit, thanks

matthijskooijman · 2019-03-07T14:02:50Z

arduino-core/src/processing/app/BaseNoGui.java

+    if (!recentlyUsedBoards.contains(targetBoard)) {
+      recentlyUsedBoards.add(targetBoard);
+    }
+    if (recentlyUsedBoards.size() > 4) {


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

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

facchinm · 2019-03-08T08:44:58Z

Hi @matthijskooijman ,
thanks for the review, much appreciated. I'm rebasing the commit on master and addressing some of your concerns in the process 😉

@sandeepmistry

This commit is part of arduino#7120 by @sandeepmistry

The list appears at the top of Board submenu Boards are also reachable via a CTRL+SHIFT+number (starting from 1)

ArduinoBot · 2019-05-14T14:31:26Z

✅ Build completed.

Please test this code using one of the following:

⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-856-linux32.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-856-linux64.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-856-windows.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-856-macosx.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-856-linuxarm.tar.xz

ℹ️ The linuxarm build is still experimental and may not be always available.

matthijskooijman

I did another review, but I think most of my previous comments are still unresolved? I also found some new things that I missed previously (or that you added, not sure).

matthijskooijman · 2019-09-19T15:39:17Z

app/src/processing/app/Base.java

+    try {
+      rebuildRecentBoardsMenu();
+    } catch (Exception e) {
+      // fail silently


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

matthijskooijman · 2019-09-19T15:43:42Z

app/src/processing/app/Base.java

+      recentLabel.setEnabled(false);
+      boardMenu.add(recentLabel);
+      rebuildRecentBoardsMenu();
+      //rebuildRecentBoardsMenu(null);


This commented line can be dropped?

matthijskooijman · 2019-09-19T15:47:53Z

app/src/processing/app/Base.java

@@ -118,6 +118,11 @@
  Editor activeEditor;

  private static JMenu boardMenu;
+  private static ButtonGroup boardsButtonGroup;
+  private static ButtonGroup recentBoardsButtonGroup;


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?

matthijskooijman · 2019-09-19T15:48:13Z

app/src/processing/app/Base.java

+    while (btns.hasMoreElements()) {
+      AbstractButton x = btns.nextElement();
+      if (x.isSelected()) {
+        return;


Why this return?

matthijskooijman · 2019-09-19T15:50:16Z

app/src/processing/app/Base.java

@@ -1367,7 +1367,7 @@ public void rebuildRecentBoardsMenu() throws Exception {
              buttonGroupsMap,
              board, board.getContainerPlatform(), board.getContainerPlatform().getContainerPackage());
      boardMenu.insert(item, 3);
-      item.setAccelerator(KeyStroke.getKeyStroke('0' + index,
+      item.setAccelerator(KeyStroke.getKeyStroke('1' + index,


This seems an unrelated off-by-one bugfix that should have been squashed into the previous commit?

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

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.

matthijskooijman · 2019-09-19T15:52:15Z

app/src/processing/app/Base.java

-    }
+    JMenuItem recentLabel = new JMenuItem(tr("Recently used boards"));
+    recentLabel.setEnabled(false);
+    boardMenu.add(recentLabel);


This also seems like a change unrelated to the click after startup commit that is better squashed into the previous commit.

matthijskooijman · 2019-09-19T15:55:05Z

app/src/processing/app/Base.java

-        if (!first)
-          boardMenu.add(new JSeparator());
-        first = false;
+        boardMenu.add(new JSeparator());


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?

matthijskooijman · 2019-09-19T15:57:41Z

app/src/processing/app/Base.java

+    while (btns.hasMoreElements()) {
+      AbstractButton x = btns.nextElement();
+      boardMenu.remove(x);
+    }


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?

ArduinoBot · 2020-04-08T13:03:58Z

✅ Build completed.

Please test this code using one of the following:

⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-950-linux32.tar.xz
⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-950-linux64.tar.xz
⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-950-windows.zip
⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-950-macosx.zip
⬇️ https://downloads.arduino.cc/javaide/pull_requests/arduino-PR-8607-BUILD-950-linuxarm.tar.xz

ℹ️ The linuxarm build is still experimental and may not be always available.

facchinm force-pushed the ide-1.9.x-beta branch from caa2b16 to 3de8e41 Compare March 7, 2019 13:30

matthijskooijman reviewed Mar 7, 2019

View reviewed changes

arduino deleted a comment from ArduinoBot Mar 8, 2019

facchinm added 5 commits March 8, 2019 10:08

Refactor boardMenu creation

0096b38

This commit is part of arduino#7120 by @sandeepmistry

Add per-session recently used boards list

94f6bb5

The list appears at the top of Board submenu Boards are also reachable via a CTRL+SHIFT+number (starting from 1)

Fix concurrent access to menuItemsToClickAfterStartup

9083508

Make Recently used boards size configurable from preferences

b21cfa0

Prepare for LRU persistency

c0710e0

facchinm force-pushed the supercool_recent_boards branch from a5ea265 to c0710e0 Compare March 8, 2019 10:30

facchinm force-pushed the ide-1.9.x-beta branch from 3de8e41 to c5ab4e3 Compare March 8, 2019 10:45

facchinm force-pushed the ide-1.9.x-beta branch from c5ab4e3 to ea8755d Compare March 18, 2019 09:52

facchinm added this to the Release 1.8.10 milestone Apr 18, 2019

per1234 mentioned this pull request May 7, 2019

[Tools>Board] and [Sketch>Include Library] split ridiculous long menus into sub-menus #8858

Open

facchinm force-pushed the ide-1.9.x-beta branch from ea8755d to 8502a9e Compare May 14, 2019 13:37

facchinm changed the base branch from ide-1.9.x-beta to master May 14, 2019 14:05

arduino deleted a comment from ArduinoBot May 14, 2019

per1234 mentioned this pull request May 27, 2019

Cleanup boards menu #8919

Closed

per1234 mentioned this pull request Jul 5, 2019

Acessibility: Refactor UI to provide alternative to dropdown menus #9037

Open

per1234 mentioned this pull request Sep 5, 2019

Offer last 7 used boards in selection menu #9197

Closed

matthijskooijman mentioned this pull request Sep 19, 2019

Divide board menu into submenus #9238

Merged

matthijskooijman reviewed Sep 19, 2019

View reviewed changes

cmaglie modified the milestones: Release 1.8.10, Release 1.8.11 Nov 8, 2019

per1234 mentioned this pull request Dec 7, 2019

[IDE] recent used boards list #9508

Closed

cmaglie modified the milestones: Release 1.8.11, Release 1.8.12 Jan 31, 2020

cmaglie modified the milestones: Release 1.8.12, Release 1.8.13 Feb 12, 2020

facchinm force-pushed the supercool_recent_boards branch 2 times, most recently from cd41ab5 to c0710e0 Compare April 8, 2020 12:44

per1234 mentioned this pull request May 6, 2020

Enhancement request for the IDE: Favourites in the list of boards. #10162

Closed

ricardojlrufino mentioned this pull request May 14, 2020

[Arduino] Las Boards Used OpenDevice/OpenDevice#182

Open

cmaglie modified the milestones: Release 1.8.13, Release 1.8.14 Jun 19, 2020

facchinm mentioned this pull request Sep 30, 2020

Implement "Recently Used Boards" Menu #10816

Open

per1234 mentioned this pull request Feb 5, 2021

recent used boards list #11246

Closed

per1234 mentioned this pull request Nov 1, 2021

Support customization of board selection drop-down arduino/arduino-ide#86

Open

3 tasks

This was referenced Mar 12, 2023

Add an option to favorite some boards arduino/arduino-ide#1953

Open

add a frequent flyer list to the boards menu arduino/arduino-ide#330

Open

per1234 mentioned this pull request Jan 21, 2024

Disabling unused boards in the IDE arduino/arduino-ide#2339

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-session recently used boards list #8607

Add per-session recently used boards list #8607

facchinm commented Mar 6, 2019 •

edited by per1234

Loading

ArduinoBot commented Mar 7, 2019

matthijskooijman left a comment

matthijskooijman Mar 7, 2019

matthijskooijman Mar 7, 2019

matthijskooijman Sep 19, 2019

rin67630 May 7, 2020

matthijskooijman Mar 7, 2019

matthijskooijman Mar 7, 2019

matthijskooijman Mar 7, 2019

matthijskooijman Mar 7, 2019

matthijskooijman Mar 7, 2019

facchinm Mar 8, 2019

matthijskooijman Mar 7, 2019

facchinm Mar 8, 2019

facchinm commented Mar 8, 2019

ArduinoBot commented May 14, 2019

matthijskooijman left a comment

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

facchinm Sep 20, 2019

matthijskooijman Sep 20, 2019

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

matthijskooijman Sep 19, 2019

ArduinoBot commented Apr 8, 2020

Add per-session recently used boards list #8607

Are you sure you want to change the base?

Add per-session recently used boards list #8607

Conversation

facchinm commented Mar 6, 2019 • edited by per1234 Loading

ArduinoBot commented Mar 7, 2019

matthijskooijman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facchinm commented Mar 8, 2019

ArduinoBot commented May 14, 2019

matthijskooijman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArduinoBot commented Apr 8, 2020

facchinm commented Mar 6, 2019 •

edited by per1234

Loading