Implement "Recently Used Boards" Menu #10816
Conversation
|
Hi @NPoole , |
There was a problem hiding this comment.
Yeah, I really want to get this feature merged. There was indeed already a PR, but it was not quite ideal and had to be refactored after my board submenu change from a while back, so I'm all for going ahead with this PR. This PR also seems to be signficantly simpler, possibly because it cleverly reuses the existing board menu items and actions.
Overall, it looks good to me. I left a few inline suggestions for improvement.
One significant question I have, is how this should handle board menus. Currently, only the board id itself is saved, AFAICS. However, when you select e.g. a Nano, then select the 168 version instead of the 328 version, then switch to another board and then switch back to Nano in the recent boards list, you might expect that it remembers that you used the 168 version.
Doing this would also make this recent boards list a lot more powerful, especially for boards with a lot of options, such as the ESP series, or the STM32 core that uses the "board" selection to select a family and selects the actual board using a board option.
To implement this, I guess whenever options are selected, the full fqbn including options must be put into the recent list, with some extra care to only compare the basic board id part of the fqbn when removing a board from the recent boards list.
This does mean that when you use a single board with different options, it will only end up in the recent list twice, but that seems acceptable to me. If you would want to include it twice, you would have a risk of including even more entries for the same board (if you need to set more than one option, can be mitigated by only actualy setting the options when you verify or upload, but that will lead to confusion). It is also quite non-trivial to come up with a proper label for the recent boards menu that allows distinguishing the same board with different options, so I think just including each board once (but do remember the last-used options) is the better option here.
Note that this same expectation could be had from the regular boards menu, but there you could easily expect that selecting the same board as earlier gets you the same default settings again. When selecting a board from a "recently used boards" list, I would be more inclined to expect to also select the settings for the board that I recently used.
| if (!recentBoard.equals(currentBoard)) { | ||
| newRecentBoardIds.add(recentBoard); | ||
| } | ||
| } |
There was a problem hiding this comment.
Couldn't this just use Collection.remove to remove currentBoard rather than manually looping? Might need to make a copy of the collection first, though just modifying the existing collection might be even shorter.
There was a problem hiding this comment.
Or maybe this would be a good usecase for this new java streaming collection API, where you could do something like "get the current list, filter out the selected board, prepend the current board, and limit to n elements". Not sure about exact syntax, but I think this could very well end up concise and readable.
There was a problem hiding this comment.
I'll look into that. I'm actually not that familiar with Java, this was my first crack at it. I suspect there are a lot of optimizations to do with collections since I mostly treated them like arrays.
There was a problem hiding this comment.
I can't figure out the optimization for this without breaking it.
There was a problem hiding this comment.
I implemented this in my branch: matthijskooijman@818c3fe
app/src/processing/app/Base.java
Outdated
| JMenuItem itemClone = new JMenuItem(actionClone); | ||
|
|
||
| // populate list of menuitem copies | ||
| recentBoardItems.put(boardId, itemClone); |
There was a problem hiding this comment.
As an optimization, maybe you could put item in recentBoardItems (which probably needs to be renamed, then) and then create the clone action and item in rebuildRecentBoardsList so you only need to create 5 of them, rather than tens or hundreds.
There was a problem hiding this comment.
The problem I have is that during createBoardMenusAndCustomMenus() I don't know what is going to be on the recent boards list, and the list is updated more often than createBoardMenusAndCustomMenus() is called. Whether I save the item, a copy of the item, or just the action, I need to save one for every board that's installed. There are only two solutions I can imagine:
-
Add a function that drills down through all installed packages and calls
createBoardMenusAndCustomMenus(). This new function would probably be called inrebuildRecentBoardsList(). I think this would significantly slow down switching boards or ports so probably not a good idea. -
Instead of cloning and saving the menu items, retrieve them from the menu during
rebuildRecentBoardsList(). Presumably, they live somewhere? I spent a long time trying to figure out how to do this... never did.
There was a problem hiding this comment.
Whether I save the item, a copy of the item, or just the action, I need to save one for every board that's installed.
Yes, I was suggesting you still have a map that contains an item for each board in the entire menu, but you wouldn't have to create the clone item and action for each, just for the ones you're actually going to use.
You could probably also find them without the map, but just using the map is probably a lot faster then trying to find them from the menus.
There was a problem hiding this comment.
I implemented this locally, cleans up things nicely. However, some other small improvements made broke things, so I haven't pushed this anywhere yet, no time to fix that now, so I'll come back to this. Also the custom board menu stuff might also need some further changes here.
There was a problem hiding this comment.
I implemented this in my branch matthijskooijman@a9631da
Note that when stuff is refactored by saving fqbn's, as I suggest in #10887, this might end up different again, though.
- Make number of recent boards adjustable in the preferences.txt file at "recent.num_boards" - Disable feature by entering value '0' for num_boards - Change Recent board menu items to radio buttons - Add "All Installed Boards" header to Boards menu -Remove unnecessary LinkedList creation at line 1599 - Broaden checks to anticipate alterations to preferences
|
Sorry for letting this languish for so long, I had other work to attend to. I've addressed most of your comments, I think, in the latest commit.
This is true of my particular changes, but the board custom menu options are already retained as part of the existing functionality.
I agree. This functionality already exists, in my testing. [Edits after re-reading thread] |
There was a problem hiding this comment.
Sorry for letting this languish for so long, I had other work to attend to.
Don't worry, I know the position all too well (and have plenty of other work to attend to in the meanwhile :-p)
Your changes look good, I left some more inline comments wrt the new config directive.
This is true of my particular changes, but the board custom menu options are already retained as part of the existing functionality.
Right, I did notice this before, though it seemed a bit erratic before. Did you look into the code how it works exactly? I have the impression that maybe it just saves the values of all options, even those not applicable to the current board, so that when you you switch form e.g. the nano with cpu=atmega168 selected to the uno which has no options, and then back, it has remembered cpu=atmega168. But then I'm afraid that if you switch to another board that does have a cpu option, change the cpu options for that board, and switch back to the nano through the recent boards menu, then it would still have the cpu option of the new board, rather than the old board.
In some cases, this might actually be what you want, e.g. when you switch from one ESP board to another, then enable the debug "board option" (which is more of a core option, really), and then switch back to the first ESP board. OTOH, maybe this should be solved by making core options core options and leaving board options board options (i.e. arduino/arduino-cli#922).
Responding to what you wrote before editing,
I can't think of a clean way to represent the different permutations in the menu. Some boards have 4 or more custom menus so while it's not totally outlandish to image an entry like:
"Arduino Pro or Pro Mini [ATmega328P (5V 16MHz)]"
You would also expect entries in excess of:
"SparkFun Artemis Module[SparkFun Variable Bootloader(Recommended),921600]"
This would be even more of a problem with cores like the ESP8266, which have even more options. i.e. here's what the IDE shows in the statusbar for such a board:
But indeed, I think we agree that including the same board with different options is not feasible, so let's not :-)
Wrt the two optimizations I suggested before, I'll have a look myself to see if I can manage to implement those. If I do, ok if I just add a commit to your branch?
| int numBoards = 0; | ||
|
|
||
| if (PreferencesData.has("recent.num_boards")) { | ||
| numBoards = PreferencesData.getInteger("recent.num_boards"); | ||
| } |
There was a problem hiding this comment.
getInteger can take a default argument, so this can be:
| int numBoards = 0; | |
| if (PreferencesData.has("recent.num_boards")) { | |
| numBoards = PreferencesData.getInteger("recent.num_boards"); | |
| } | |
| int numBoards = PreferencesData.getInteger("recent.num_boards", 0); |
But, given what I say about the default preferences.txt in another comment, I think the default can even be left out:
| int numBoards = 0; | |
| if (PreferencesData.has("recent.num_boards")) { | |
| numBoards = PreferencesData.getInteger("recent.num_boards"); | |
| } | |
| int numBoards = PreferencesData.getInteger("recent.num_boards"); |
There was a problem hiding this comment.
I did this in my branch, as part of some other cleanup.
| // If recent.num_boards is 0, interpret this as the feature | ||
| // being turned off. There's no need to rebuild the menu | ||
| // because it will be hidden | ||
| if (numBoards > 0) { |
There was a problem hiding this comment.
I believe this if could be broader: If numBoards == 0, then there's no point to even set up, add to and prune the list at all. It might still be needed to create an empty list to keep other code happy, but maybe not even.
There was a problem hiding this comment.
In my branch, I ended up just removing this if instead, since the code should produce the right result in that case anyway (it is slightly slower, but having this disabled is a corner case that should not really be optimized for).
| // Check if the field exists, in case preferences got lost | ||
| if (!PreferencesData.has("recent.num_boards")){ | ||
| // (default to 5) | ||
| PreferencesData.setInteger("recent.num_boards", 5); | ||
| } |
There was a problem hiding this comment.
I think that setting default values for preferences like this is not normally done, instead it should be added to build/shared/lib/preferences.txt, which is always loaded before the user's preferences.txt IIRC, so that sets the default value for when the user's file does not contain the preference (which is then saved to the user's preferences.txt on quit).
So this could just be removed.
| // Check if the field exists, in case preferences got lost | |
| if (!PreferencesData.has("recent.num_boards")){ | |
| // (default to 5) | |
| PreferencesData.setInteger("recent.num_boards", 5); | |
| } |
I looked at this, it seems that the core of this code is here and indeed mixes up settings for different boards and/or cores and is a bit of a mess in general. I'm halfway through writing an issue that points out some of the problems with the current approach, that could maybe be solved together with this PR, so I'll come back to this somewhere next week. |
Please feel free! |
|
See #10887 for the issues I had with the custom boards handling. It also makes the following suggestion to fix part of those inconsistencies:
How does that sound? I also noticed that the implementation in this PR currently only stores bare board ids, excluding the package or platform (i.e. it stores Because that issue and this PR have some interaction, it might be useful to fix them together (or at least not merge the recently used boards support in one way, and then change how it works wrt to board menus later).
Hm, seems I cannot, I think you unchecked the "Allow contributors to push to my branch" or something like that when creating the pullrequest. So, I pushed to my own branch instead, feel free to pull from there: https://github.com/matthijskooijman/Arduino/commits/recent-boards I just added some new commits, I think most of the commits can be squashed later before merging this. |
|
|
This PR adds a "Recently Used Boards" section to the "Boards" submenu. The effect of this change is to add a list of 5 menu items to the "boards" submenu which represents the 5 most recently selected boards. When clicked, these menu items select the board in question. An empty "Recently Used Boards" section will never appear and will only render after a board has been selected by the user, populating the
recent.boardspreferences key.Implementation
rebuildBoardsMenu(), check to see whether preferences hasrecent.boards. If so, insert separators and a menu label to denote the "Recently Used Boards" list and store the index of this section for the insertion of menu items later.createBoardMenusAndCustomMenus(...)create a collection of 'clone' menu items whose action is to click the original menu items. Reference them by boardId.onBoardOrPortChange()update the list of boardIds inrecent.boards. This list is treated as a stack with new boards popping off the oldest member of the list. Then callrebuildRecentBoardsList()to remove previous menu items and insert new menu items using the collection generated duringcreateBoardMenusAndCustomMenus(...).Notes
JMenu boardMenu