[IDE] Slow Startup - BaseNoGui.onBoardOrPortChange trigger two rescanLibraries()

[ricardojlrufino]

I noticed that the method is being called 2 twice:
librariesIndexer.rescanLibraries ()

I believe this is a bug, because the librariesIndexer.setArchitecturePriority, seems to have no influence on the process.

This makes the other issue (#10214) I found a little worse

[matthijskooijman]

I believe this is a bug, because the librariesIndexer.setArchitecturePriority, seems to have no influence on the process.

But isn't this fixed by calling rescanLibraries the second time? Maybe that's why it is called the second time at all?

Scanning twice is of course a performance problem to be fixed. Without looking at the code in detail, I would think that maybe setLibrariesFolders should just not call rescanLibraries()? It seems that the two places that call setLibrariesFolders now also call rescalLibraries, so just removing the first call would be ok?

[cmaglie]

After looking at the call tree of setLibrariesFolder, the following places setLibrariesFolder is followed by rescanLibraries:

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

Lines 365 to 366 in 02a2e22

	indexer.setLibrariesFolders(BaseNoGui.getLibrariesFolders());
	indexer.rescanLibraries();

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

Lines 674 to 681 in 02a2e22

	// Scan for libraries in each library folder.
	// Libraries located in the latest folders on the list can override
	// other libraries with the same name.
	librariesIndexer.setLibrariesFolders(librariesFolders);
	if (getTargetPlatform() != null) {
	librariesIndexer.setArchitecturePriority(getTargetPlatform().getId());
	}
	librariesIndexer.rescanLibraries();

Arduino/app/test/cc/arduino/contributions/UpdatableLibraryTest.java

Lines 39 to 40 in 02a2e22

	indexer.setLibrariesFolders(folders);
	indexer.rescanLibraries();

Arduino/app/test/cc/arduino/contributions/UpdatableLibraryTest.java

Lines 66 to 67 in 02a2e22

	indexer.setLibrariesFolders(folders);
	indexer.rescanLibraries();

---

Instead in the following places the "autoreload" feature is actually used:

Arduino/app/test/cc/arduino/contributions/UpdatableLibraryTest.java

Lines 48 to 49 in 02a2e22

	folders.add(new UserLibraryFolder(SD121, Location.SKETCHBOOK));
	indexer.setLibrariesFolders(folders);

Arduino/app/test/cc/arduino/contributions/UpdatableLibraryTest.java

Lines 75 to 76 in 02a2e22

	folders.add(new UserLibraryFolder(Bridge170, Location.SKETCHBOOK));
	indexer.setLibrariesFolders(folders);

---

@ricardojlrufino
if you add a call to rescanLibraries in the two cases above, I think you can safely remove the rescanLibraries call from setLibrariesFolder. Nice catch, if you want to open a PR this can be quickly merged.

[matthijskooijman]

Instead in the following places the "autoreload" feature is actually used:

Ah, I had not looked at the tests in my analysis above, good call. But indeed, just calling rescalLibraries in the tests seems fine :-)

[ricardojlrufino]

to have something cleaner, what about adding a second boolean parameter to setLibrariesFolders
like : indexer.setLibrariesFolders(File files, boolean rescan);
what do you think ?

[ricardojlrufino]

[matthijskooijman]

That would also be ok (though boolean parameters have the risk of becoming unclear if you don't add a comment in the call), but I'm not so sure it would be needed if rescanLibraries is called always anyway (ignoring the tests). OTOH, in one of the two places in the code, rescanLibraries is called directly after setLibrariesFolders, so that could be shortened to pass true instead).

Still, I'm not sure. On one hand, it might be good to somewhat ensure that rescan happens when changing the library folder. OTOH, explicitly calling rescanLibraries() might result in code that is easier to read (clearer what is going on).

One other alternative would be to have a setLibrariesFolderAndRescan() function, which would be easier to read that a true argument.

[cmaglie]

I agree, we already have made some API in the IDE in the past using boolean that lead to methods like doSomething(true, false, false). That looks very bad in retrospecitve.

@ricardojlrufino I'd follow Matthijs suggestion and do:

• add rescanLibraries() where it's missing and remove it from setLibrariesFolder()

or

• add setLibrariesFolderAndRescan() and remove rescan from setLibrariesFolder()

up to you, both looks OK to me.