Linux: Removed X11 requirement when running in command line mode #5578
Conversation
cmaglie
added
Component: IDE
|
✅ Build completed. Please test this code using one of the following: ⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5578-BUILD-614-linux32.tar.xz ℹ️ The |
|
Just tested PR-5578-BUILD-614 on Mac OS 10.10.5 and Debian 8.6 Linux 32-bit.
Working building of examples using Jenkins: |
|
These changes work well on the WiFi101 Travis CI build: https://travis-ci.org/sandeepmistry/WiFi101/builds/174214198 - no more |
There was a problem hiding this comment.
Great changes, good to finally solve this. I do see that this is mostly a pragmatical approach to remove the GUI dependencies. For a more conceptual solution, only stuff from arduino-core should be used and the separation between Base and BaseNoGui should be made more sharp, but we can just make steps in that direction when there is opportunity in the future.
One overall comment: Some of the commit message first lines end in a period, which is not the convention :-)
| BaseNoGui.initParameters(args); | ||
|
|
||
| splashScreenHelper.splashText(tr("Loading configuration...")); | ||
| CommandlineParser parser = new CommandlineParser(args); | ||
| parser.parseArgumentsPhase1(); |
There was a problem hiding this comment.
Now parseArgumentsPhase1 is moved up this far, it might be good to move it even further up so BaseNoGui.initParameters can just use the --preferences-file value as parsed by parser, instead of having to parse it iself. From a quick glance of parseArgumentsPhase1, it seems this does not depend on the preferences file being loaded? This should probably be a different commit, of course.
There was a problem hiding this comment.
Unfortunately parseArgumentsPhase1 depends on preferences being loaded, it uses PreferencesData in 3 or 4 places. Of course this smells like a bad design, the CommandLineParser class should only parse the command line and actions should be taken outside. I guess it's the result of trying to keep low the amount of changes involved in previous refactorings.
(again, while doing this PR I tried also to fix this one, but dropped this task to focus on solving the X11 issue)
There was a problem hiding this comment.
Right. Sounds like the commandline parsing needs another refactor after this one then (or, at least a parseArgumentsPhase0 to just parse the preferences path).
| File sketchbookFolder = BaseNoGui.getDefaultSketchbookFolder(); | ||
|
|
||
| if (sketchbookFolder == null) { | ||
| if (sketchbookFolder == null && !isCommandLine()) { |
There was a problem hiding this comment.
Does this not break below, when sketchbookFolder is null and sketchbookFolder.exists() is called? This seems to be a bit of a corner case, since it only occurs when the default sketchbook path (as returned by Platform.getDefaultSketchbookFolder()) is null. I'm not exactly sure how if this should not normally happen, or if this will happen if the default path does not exist (but judging from the mkdirs() a bit further down, it's probably the former).
Looking through the code to see how this works, it seems that the responsibility for resolving the sketchbook path might be too much spread out right now, so perhaps some additional refactoring should be done after this PR in this area.
There was a problem hiding this comment.
As you noticed the folders handling code is spread around the IDE, and it needs a bit of love :-) Actually, I tried to factor all the folder handling code in a single class while doing this PR, but at some point I've just dropped this task to reduce the possibility of regressions.
The rationale behind this change is:
- if my calculations are correct
BaseNoGui.getDefaultSketchbookFolder()will never returnnull - this means that the
promptSketchbookLocation()will never be called (and could be probably removed altogether)
but since I decided to not touch the folder handling functions in this PR, I opted to just leave it as it is and just add the "guard" to not invoke the file selector in cmd-line mode and let the IDE crash with a NPE (if that code path is ever taken in some obscure ways).
There was a problem hiding this comment.
Sounds reasonable.
| // Make sure these verbosity preferences are only for the | ||
| // current session | ||
| // Set preserve-temp flag | ||
| PreferencesData.setBoolean("runtime.preserve.temp.files", parser.isPreserveTempFiles()); |
There was a problem hiding this comment.
Perhaps changing set to setBoolean should end up in a separate commit?
| if (!lastStatus.equals(progress.getStatus())) { | ||
| // Reduce verbosity when running in console | ||
| String s = progress.getStatus().replaceAll("[0-9]", ""); | ||
| double p = progress.getProgress(); |
There was a problem hiding this comment.
This seems like a bit of a convoluted way to reduce verbosity here, but I'm not familiar enough with this Progress stuff to offer a better suggestion.
There was a problem hiding this comment.
The point of this change is to reduce the output of the lines like:
Downloading tools 10k of 15680k...
Downloading tools 20k of 15680k...
Downloading tools 30k of 15680k...
Downloading tools 40k of 15680k...
While this is nice in a GUI, with a progress bar etc., it's a mess on command line, this commit was the simplest way that comes to mind.
This commit makes this changes: - SplashScreenHelper is now local in Base constructor - if SplashScreenHelper is instantiated with a null SplashScreen instance then it outputs progress in console and avoid to make calls to Swing toolkit - The parsing of command line arguments is anticipated so we can determine if we are in command line or GUI mode early and setup objects that produces output to not use graphics toolkits. - In this case the SplashScreenHelper is initialized with a real splashscreen only if we are in GUI mode
Move the initialization of Editor into the GUI section of the big if-then-elseif chain. This actually breaks cases for Verify and Upload that uses Editor to access core functions. This will be fixed in next commits.
This commit concludes the refactoring.
Previously it was used to prevent the Editor from being displayed when running in command-line mode. Now the Editor is not created at all, so this parameter is useless. This is also confirmed by the remaining calls to `handleOpen` that all have the parameter set to `true`.
The properties:
System.setProperty("awt.useSystemAAFontSettings", "on");
System.setProperty("swing.aatext", "true");
actually works on Linux (where the hint helps X11 to enable antialiased
rendering) but makes things worse on Windows where the outcome is exactly
the opposite (anti-alias is disabled).
Previously those settings had no effect because they were executed
*after* the initialization of the graphics. This is no more true
after the merge of arduino#5578, that moved the graphics initialization
after commmand line parsing and consequently revealed the weird
behaviour on windows.
Fix arduino#5750
The properties:
System.setProperty("awt.useSystemAAFontSettings", "on");
System.setProperty("swing.aatext", "true");
actually works on Linux (where the hint helps X11 to enable antialiased
rendering) but makes things worse on Windows where the outcome is exactly
the opposite (anti-alias is disabled).
Previously those settings had no effect because they were executed
*after* the initialization of the graphics. This is no more true
after the merge of #5578, that moved the graphics initialization
after commmand line parsing and consequently revealed the weird
behaviour on windows.
Fix #5750
Fix #1981
This PR complete the work started in #2328 by me and @bitron and use also some refactorings made by @matthijskooijman on Editor and CommandLineParser.
The graphics elements are now created only when the IDE runs in GUI mode, this allowed to significantly reduce startup time (on my system I measured 25% less, from 1.750s to 1.350s).