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

Implement "Recently Used Boards" Menu #10816

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

Conversation

NPoole
Copy link

@NPoole NPoole commented Sep 29, 2020

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.boards preferences key.

Implementation

  • During rebuildBoardsMenu(), check to see whether preferences has recent.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.
  • During createBoardMenusAndCustomMenus(...) create a collection of 'clone' menu items whose action is to click the original menu items. Reference them by boardId.
  • During onBoardOrPortChange() update the list of boardIds in recent.boards. This list is treated as a stack with new boards popping off the oldest member of the list. Then call rebuildRecentBoardsList() to remove previous menu items and insert new menu items using the collection generated during createBoardMenusAndCustomMenus(...).

Notes

  • Broadens the scope of JMenu boardMenu
  • Adds a small collection to preferences.txt

@per1234 per1234 added the Component: IDE user interface The Arduino IDE's user interface label Sep 30, 2020
@facchinm
Copy link
Member

Hi @NPoole ,
were you aware of this #8607 ? 🙂
Anyway, your patch is by far better since it implements persistence!
The only thing I suggest is adding an accelerator on the menu entries (https://github.com/arduino/Arduino/pull/8607/files#diff-b6fda0bf6be0a53c0741a9b02ac16f37R1400) to speedup the switch between two boards

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

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);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I can't figure out the optimization for this without breaking it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I implemented this in my branch: matthijskooijman@818c3fe

app/src/processing/app/Base.java Show resolved Hide resolved
app/src/processing/app/Base.java Outdated Show resolved Hide resolved
app/src/processing/app/Base.java Outdated Show resolved Hide resolved
app/src/processing/app/Base.java Show resolved Hide resolved
JMenuItem itemClone = new JMenuItem(actionClone);

// populate list of menuitem copies
recentBoardItems.put(boardId, itemClone);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. Add a function that drills down through all installed packages and calls createBoardMenusAndCustomMenus(). This new function would probably be called in rebuildRecentBoardsList(). I think this would significantly slow down switching boards or ports so probably not a good idea.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

app/src/processing/app/Base.java Outdated Show resolved Hide resolved
- 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
@NPoole
Copy link
Author

NPoole commented Oct 23, 2020

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.

Currently, only the board id itself is saved, AFAICS.

This is true of my particular changes, but the board custom menu options are already retained as part of the existing functionality.

[...]so I think just including each board once (but do remember the last-used options) is the better option here.

I agree. This functionality already exists, in my testing.

[Edits after re-reading thread]

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

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:

image

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?

Comment on lines +1380 to +1384
int numBoards = 0;

if (PreferencesData.has("recent.num_boards")) {
numBoards = PreferencesData.getInteger("recent.num_boards");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

getInteger can take a default argument, so this can be:

Suggested change
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:

Suggested change
int numBoards = 0;
if (PreferencesData.has("recent.num_boards")) {
numBoards = PreferencesData.getInteger("recent.num_boards");
}
int numBoards = PreferencesData.getInteger("recent.num_boards");

Copy link
Collaborator

Choose a reason for hiding this comment

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

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +1503 to +1507
// 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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Suggested change
// 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);
}

@matthijskooijman
Copy link
Collaborator

Right, I did notice this before, though it seemed a bit erratic before. Did you look into the code how it works exactly?

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.

@NPoole
Copy link
Author

NPoole commented Oct 23, 2020

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?

Please feel free!

@matthijskooijman
Copy link
Collaborator

See #10887 for the issues I had with the custom boards handling. It also makes the following suggestion to fix part of those inconsistencies:

To fix this, I would consider just not preserving options at all between different boards and also not when coming back to an earlier board. IOW, whenever you select a different board, just reset all options to the default values.

One exception could be that for the recently-used-boards list (as being developed in #10816), one would probably expect to get the same options as the last time this particular board was used.

To implement this, and at the same time heavily simplify implementation, I would suggest removing the current board, target_package, targe_platform and custom_* preferences, and replace them with a single fqbn preference (as already used by arduino-builder and arduino-cli). The recently-used-boards list can then also just store a list of fqbn's and easily include all board options in there.

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 uno, not arduino:avr:uno), meaning that when the same board id is used in different platforms, they will get mixed up. This should be fixed by using the full board name, or as suggested above, the fqbn including any board menu options.

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

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?

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.

@facchinm facchinm added this to the Release 1.8.14 milestone Dec 1, 2020
@per1234 per1234 mentioned this pull request Feb 5, 2021
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: IDE user interface The Arduino IDE's user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants