Inconsistencies and unneeed complexity in board option menu handling

[matthijskooijman]

While reviewing #10816 (recently used boards), I had a closer look at the custom board menu code and I believe there's a few things about that code that are fishy and fragile, and would deserve improvement. These are a few separate, but heavily related issues, so I'm listing them in detail below.

TL;DR: I would propose to:

• Not translate menu titles
• Do not preserve board options across board changes
• Allow remembering board options for boards selected earlier only through the in-development recently-used-boards list
• Simplify the plethora of current-board-related preferences and use a single fqbn property instead

One overall remark (that I made previously here), is that maybe most of this code can be replaced by code that just lets arduino-cli figure out various options and menus available somewhere in the near future. If this code is refactored now, that option should certainly be considered as well, so any work done now can be somewhat smoothly migrated to that later.

Menu titles are translated

The titles of board menus, as defined in boards.txt, are translated. This makes some sense for Arduino-supplied cores, where Arduino can both control the boards.txt entries and the translations in the IDE, but for third-party boards, which have no control over the translation, this will result in untranslated entries, mixed entries (some translated, some untranslated) or even incorrect translations (when the board menu use a word that is also used in the IDE, but in a different context).

I would suggest just not translating these titles, just like we did with platform names before (see #9238 and e003049).

See:

Arduino/app/src/processing/app/Base.java

Line 1596 in a056a5f

JMenu menu = getBoardCustomMenu(tr(title), getPlatformUniqueId(targetPlatform));

Arduino/app/src/processing/app/Base.java

Line 1472 in a056a5f

JMenu customMenu = new JMenu(tr(customMenuTitle));

Comparing titles to look for options

Board menu JMenuItems are created in one place and then looked up in another place. This uses the menu title against the JMenuItem label to find the right one, which is fragile. If two different options within the same core would use the same title (which makes no real sense, but could happen), then options of both would be mixed in the same menu I think. Better to use the id of the menu instead (by storing the id as a client property, or maybe use an (id,platform)-to-menu-item map rather than searching a long list).

See:

Arduino/app/src/processing/app/Base.java

Line 1596 in a056a5f

JMenu menu = getBoardCustomMenu(tr(title), getPlatformUniqueId(targetPlatform));

Arduino/app/src/processing/app/Base.java

Line 1668 in a056a5f

if (label.equals(menu.getText()) && menu.getClientProperty("platform").equals(platformUniqueId)) {

Arduino/app/src/processing/app/Base.java

Line 1472 in a056a5f

JMenu customMenu = new JMenu(tr(customMenuTitle));

Boards array only contains one value ever?

In this bit of code, a boards array is created, possibly with the intent of collecting all boards that share a given option? However, AFAICS, this always results in an array with just one item every, since subAction is always a new object, so subAction.getValue("board"); will always return null?

Arduino/app/src/processing/app/Base.java

Lines 1602 to 1613 in a056a5f

	Action subAction = new AbstractAction(tr(boardCustomMenu.get(customMenuOption))) {
	public void actionPerformed(ActionEvent e) {
	PreferencesData.set("custom_" + menuId, ((List<TargetBoard>) getValue("board")).get(0).getId() + "_" + getValue("custom_menu_option"));
	onBoardOrPortChange();
	}
	};
	List<TargetBoard> boards = (List<TargetBoard>) subAction.getValue("board");
	if (boards == null) {
	boards = new ArrayList<>();
	}
	boards.add(board);
	subAction.putValue("board", boards);

In practice, only the first element of boards is used anyway, it seems:

Arduino/app/src/processing/app/Base.java

Line 1604 in a056a5f

PreferencesData.set("custom_" + menuId, ((List<TargetBoard>) getValue("board")).get(0).getId() + "_" + getValue("custom_menu_option"));

Saving custom values to preferences.txt is fragile and leads to seemingly arbitrary saving and forgetting of custom options

Currently, custom menu options are stored like custom_optionname=boardid_optionvalue. I suppose that including the boardid in the value is intended to prevent preserving values from one board to another or something? Or maybe the boards array mentioned above was originally intended to canonicalize this board id to the first boardid within a platform, to allow preserving options between boards in a platform but not outside? In any case, AFAIU the current behavior is that:

• When you select some options for one board, then switch to another board, then any shared options will be overwritten with values for the new board.
• When you come back to the original board, the overwritten options will not match the board id, so be reset to the defaults again and overwritten. However, any options that were not overwritten (because the other board does not support them), will be kept, so their old values are used.
• In effect, this means that while switching between boards, coming back to an earlier board, some options will be preserved and some will not, rather arbitrarily.

Also note that the boardid used in the custom_... options in preferences is the unqalified board id, so if two platforms contain a board by the same name, then switching between them will preserve options, which makes no sense to me.

See also:

Arduino/app/src/processing/app/Base.java

Line 1604 in a056a5f

PreferencesData.set("custom_" + menuId, ((List<TargetBoard>) getValue("board")).get(0).getId() + "_" + getValue("custom_menu_option"));

Using _ as a separator is fragile, since board ids might also contain underscores. However,, it seems that when loading these preferences, only very primitive board id matching is used, not even checking the _ after the board id:

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

Line 149 in a056a5f

if (entry != null && entry.startsWith(boardId)) {

In practice, this means that a custom option stored for one board, e.g. custom_opt=myboardv3_foo would be applied to another board as well, but in a broken way (e.g. for the myboard board, this would apply the value 3_foo).

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.

For options which are shared between a lot of different boards, saving options could be a useful addition (does not work right now), but in practice most of these options are really just core options (debug level, flash layout, etc.) or upload options (upload speed) and should probably be treated as such (i.e. see arduino/arduino-cli#922).

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.

The code is complex and slow

The creation of these board menus is quite complex. IIUC, whenever the boards menu is rebuilt, all possible board menus for all possible platforms are created with all possible options for all boards, most of which will be invisible most of the time. Loading preferences happens by remembering items to click later, which is not elegant and probably leads to running some things repeatedly (such as this call). This slowness was already discussed in #10214 before.

I haven't looked closely yet, but maybe just all of this code should be replaced by something that is a lot simpler and just regenerates the custom menu options whenever the current board changes?

[matthijskooijman]

I found another weirdness related to this. When

• the platform for the currently selected board is removed, but the same plain board name exists with the same options in another platform
• or easier, when the platform is renamed from e.g. foo/avr to bar/avr
• or you remove a platform through the board manager that is also installed into your sketchbook manually,
  and then the IDE is started, the logic that makes sure to select a board when none is selected does not fire. This is because it fires only when menuItemsToClickAfterStartup is empty, but in this case that list contains custom menu options, but not an actual board option. This then cause no board to be selected, and with rebuildProgrammerMenu: Handle no current board #10922 applied to fix the resulting null pointer exception, all custom menus end up being visible...