Editor cleanup #4363
Editor cleanup #4363
Conversation
|
It is sad to see some of the best things in the original codebase going away without even having been noticed for how useful they were. To be clear, many of the improvements i made, at times with minimal amount of work, would become the bigger efforts that some of the core team mentionned in previous comments. The positive news is that you will no longer have to deal with my comments as this codebase is no longer worth my time. |
|
I just got the test suite working, which showed a few tests were failing due to a problem with the caret position saving and undo menu items. I added two commits to fix that, which I'll later merge into the appropriate earlier commits (but leaving them separate is easier to review). |
|
@lmihalkovic - You speak of "many of the improvements i made", but all your public activity appears to only rhetoric, without any real code contributions. |
|
@PaulStoffregen, that is a job for other people, apparently |
|
@PaulStoffregen regarding the debugger you are writting, the impact these changes will have on your work (once you've evaluated it), I thought you'd be one of the people understanding (#4010) @Chris--A I pasted the exact working solution to the folding issue from my codebase (very different from yours) and even explained what should not need any explanations. Clearly one of you can write the handfull of lines of code to adapt it to your codebase @matthijskooijman it is only in the software industry that people are denied an opinion unless they are willing to redo the work. Personally when someone tells me I am clearly not considering the full impact of what i am doing, I have a critical look at my work, not the messenger. This patch closes the doors it closes regardless of who makes the evaluation, and I live comfortably with the knowledge that for the issues I saw, other more experienced developpers will undoubtedly see more. Issues live in source code, not in the eyes of the beholder |
|
@matthijskooijman The changes you propose seem great, but their number looks massive to me... What would be the best way to go forward ? My contribution might not be of very high value as I'm new to the IDE code, but I could review them all one-by-one, learning along the way. Would it help ? If not, how can I (how can we) help ? |
|
Some perspective might be useful. In june 2015 #3441 was added to this repo regarding the editor loosing the state of foldings when switching around between tabs. An assessment was made by a member of the core team that: For the better part of 6 months this remained the only voiced opinion on the topic and nothing further happened. Reading the comment I thought it was clearly the wrong assessment, which was also missing an obvious opportunity. To pre-empt another of @PaulStoffregen's rethoric only comments, I pasted my code as-is, even adding a redundant explanation to further reduce the efforts for anyone adapting my solution to this codebase. #3441 is a small example of what I regularly refer to as low hanging fruits. Regardless of personal opinions, it might prompt at least one person to start thinking that maybe even just out of sheer luck, what I say about the existence of many such opportunities to drastically improve this application at low cost might have some degree of truth, that some of them might actually be as trivial as #3441 is, and that I might even have a reasonable enough sense of software development to actually be meaningfully inviting you to consider some of the further consequences of this patch... regardless of when I publish my own fork. |
|
@lmihalkovic I read you, Laurent, and I can understand your frustration that it took a long time to fix a bug you proposed a solution for. But the fact is that we now have a proposed version that fixes that bug (agreed, not using your code but as part of a more massive refactoring), so I think the only constructive move is to help to have it merged into the tree. If there is a good reason for not merging it (regression), let's find it and fix it. What best way to start a new year ? :-). KR. Vicne |
|
@vicnevicne, review of these commits would certainly be helpful. Even if you're not familiar with the code, just flagging things that seem strange or wrong to you is fine (easiest is to just comment on the commits themselves). Some things might look wrong, but will be fixed later, but don't let that stop you from flagging things, it's probably counter-productive to read through all commits before commenting :-) @lmihalkovic as for the fix you proposed, I'll add a coment to #3441 about that, to keep the discussion about that in that issue. |
|
@vicnevicne i appreciate your comment and the effort it reflects to try to touch on these issues. |
@lmihalkovic |
|
IMHO, everything starts with the vision Arduino LLC has for its software products. You have an existing multi-year investment in IDE and a growing stake in Create. If the immediate future is to let IDE go into sunset mode and let the feature gap grow, then nothing can be wrong about any changes and this is as good a patch as any. If on the other hand there are some features that Arduino LLC would like to schedule as complements or stepping stones or even vendor locking into Create, then any big refactoring should be evaluated in terms of how much closer it gets you to these goals, rather than how easy or trend compliant it is. |
matthijskooijman
force-pushed
the
editor-cleanup
branch
from
c325916
to
6e5b789
Compare
|
I just added three more commits that fix up some things in previous commits. I'll make sure to merge these into the commits they fix before merging this PR, but I'll keep them separately for now for easier review. |
matthijskooijman
force-pushed
the
editor-cleanup
branch
from
6e5b789
to
2565e84
Compare
For some toolbar buttons, when it is clicked while shift is pressed, its function changes. When handling the click event, this information is directly taken from KeyEvent.isShiftDown(). However, to also show the proper tooltip *before* clicking, EditorToolbar listened to key events on the main text area, to know when shift is (not) pressed. This approach means that pressing shift while the text area is not focused will not change the tooltip, and creates some unwanted coupling between the toolbar and the text area. This commit changes this approach to instead use the global KeyboardFocusManager. Any key presses pass through there before being dispatched to the currently focused component, so this makes sure that any shift presses are caught, as well as making EditorToolbar a bit more self-contained.
RSyntaxTextArea appears to support using a single instance and replacing the underlying text and document when switching between tabs, but in practice this support is not complete and even though the RSyntaxTextArea developers did some work to improve the situation, they recommend to just use a seperate instance for each tab. This commit implements exactly that. A new class EditorTab is introduce to wrap the RSyntaxTextArea and containing scroll pane, and to encapsulate the code related to handling the text area itself. Doing so removes some quirks and prepares for some later additions. In particular, error highlights are now no longer shared between all tabs, which was previously the case. This commit mostly moves code from Editor into EditorTab, and updates the callers to use getCurrentTab() and call methods on the result instead of calling them on Editor. Some code is added to take care of creating multiple EditorTab objects and switching between them. Some small changes have been made to make the flow of opening files work, though these are mostly a bit hacky. While moving code, changes to the rest of the code were kept minimal, retaining existing interfaces as much as possible. This sometimes result in less than ideal code, which should be cleaned up in subsequent commits. The SketchCodeDocument class has been pretty much emptied out, since it was mostly used to store things for tabs in the background, which are now just stored in each RSyntaxTextArea separately. The last remaining bits of this class can probably be moved or implemented differently later, so it can be removed. The entire flow of working with sketches and files needs to be cleaned up next, so no thorough attempt at testing this commit was done. It is likely that there are plenty of corner cases and race conditions, which will be fixed once the reset of the code is cleaned up. Fixes arduino#3441
It was not used anymore, and removing it makes subsequent refactoring easier.
Previously, EditorTab set the Document on the SketchCodeDocument, and the latter would listen for changes, only forwarding the modified status to SketchCode. This commit cuts out a step and lets EditorTab call SketchCode::setModified directly. Additionally, the DocumentTextChangedListener helper class is added, which wraps a simple (lambda) function to be called whenever anything about the document text is modified. This hides the verbosity of having to handle both insertion and deletion, and instead suffices with just having a single lambda function instead.
Now that each file in the sketch has its own text area in the GUI, it is no longer needed to store the (possibly modified) contents of each file inside SketchCode. Keeping the contents in the text area is sufficient. Doing so allows removing the code that dealt with copying contents from the text area into the SketchCode instance at the right time, which was fragile and messy. However, when compiling a sketch, the current (modified) file contents still should be used. To allow this, the TextStorage interface is introduced. This is a simple interface implemented by EditorTab, that allows the SketchCode class to query the GUI for the current contents. By using an interface, there is no direct dependency on the GUI code. If no TextStorage instance is attached to a SketchCode, it will just assume that the contents are always unmodified and the contents from the file will be used during compilation. When not using the GUI (e.g. just compiling something from the commandline), there is no need to load the file contents from disk at all, the filenames just have to be passed to arduino-builder and the compiler. So, the SketchCode constructor no longer calls its `load()` function, leaving this to the GUI code to call when appropriate. This also modifies the `SketchCode.load()` function to return the loaded text, instead of storing it internally. To still support adding new files to a sketch (whose file does not exist on disk yet), the EditorTab constructor now allows an initial contents to be passed in, to be used instead of loading from disk. Only the empty string is passed for new files now, but this could also be used for the bare minimum contents of a new sketch later (which is now down by creating a .ino file in a temporary directory). Another side effect of this change is that all changes to the contents now happen through the text area, which keeps track of modifications already. This allows removing all manual calls to `Sketch.setModified()` (even more, the entire function is removed, making `Sketch.isModified()` always check the modification status of the contained files).
Previously, some of the GUI code would use Editor.getSketch() to get the current sketch, and Sketch.getCurrentCode() to find out the currently selected tab. Since this code is really concerned with the currently open tab in the GUI, it makes more sense to query the Editor tabs list directly. This removes all references the current sketch code, as tracked by Sketch, external to Sketch itself. This prepares for removing the current tab tracking from Sketch later.
Instead of letting Sketch (also) keep track of the currently selected tab, this moves the responsibility to Editor instead. When Sketch need to know the current tab and file, it now asks Editor. Switching between tabs is still handled through Sketch methods, but that will be cleaned up later.
This lets all code directly call `Editor.selectTab()`, or the newly introduced `Editor.selectNextTab()` or `Editor.selectPrevTab()`. This also adds a new `Editor.findTabIndex(String)` to look up a tab based on the filename (what `Sketch.setCurrentCode(String)` used to do). At some point, this method might need to be removed, but for now it allows other code to keep working with minimal changes.
This class served no purpose anymore, so it can be removed. The `SketchCode.getMetadata()` and `setMetaData()` methods only served to keep track of a SketchCodeDocument instance (and were no longer used), so these are removed too, just like some SketchCode constructors dealing with this metadata object.
It was not used, and since it only updated the `name` attribute, but not the corresponding `file` attribute, nor actually handled renaming actual files, having this method around would actually be harmful, so just drop it.
This makes a few related changes: - `FileUtils.replaceExtension()` is introduced to handle replacing the .pde extension with .ino. - Instead of iterating .pde files on disk, this iterates SketchFiles in memory, saving another lookup from filename -> SketchFile later. - `SketchController.renameCodeToInoExtension()` is removed. Now it no longer needs to look up the SketchFile and FileUtils handles the extension replacement, this method did not have any reason to exist anymore. - Instead of hardcoding the .pde extension, a new Sketch.OLD_SKETCH_EXTENSIONS constant is introduced.
No need to call the File constructor ourselves, if `File.getParentFile()` can just do that for us.
This commits replaces a significant part of the code handling these features. A lot of responsibilities are moved from SketchController to Sketch, though the code involved is rewritten mostly. Most of the handling now happens inside Sketch, including various checks against the new filename. Basically SketchController processes the user input to decide what needs to be done, and Sketch checks if it can be done and does it. If problems occur, an IOException is thrown, using a translated error message that is shown by SketchController as-is. This might not be the best way to transfer error messages (regular IOExceptions might contain less-friendly messages), so this might need further improvement later. In addition to moving around code and responsibilities, this code also changes behaviour in some places: - Because Sketch and SketchFile are now in control of renames and saves, they can update their internal state after a rename. This removes the need for reloading the entire sketch after a rename or save as and allows `Editor.handleOpenUnchecked()` to be removed. - When renaming the entire sketch, all files used to be saved before renaming, since the sketch would be re-opened after renaming. Since the re-opening no longer happens, there is no longer a need to save the sketch, so any unsaved changes remain unsaved in the editor after renaming the sketch. - When renaming or adding new files, duplicate filenames are detected. Initially, this happened case sensitively, but it was later changed to use case insensitive matching to prevent problems on Windows (where filenames cannot differ in just case). To prevent complexity, this did not distinguish between systems. In commit 5fbf962 (Sketch rename: allowig a case change rename if NOT on windows), the intention was to only do case insensitive checking on Windows, but it effectively disabled all checking on other systems, making the check not catch duplicate filenames at all. With this commit, all these checks are done using `File.equals()` instead of comparing strings, which is already aware of the case sensitivity of the platform and should act accordingly. - Some error messages were changed. - When adding a file, an empty file is not created directly, but only a SketchFile and EditorTab is added. When the sketch is saved, the file is created. - When importing a file that already exists (thus overwriting it), instead of replacing the SketchFile instance, this just lets the EditorTab reload its contents. This was broken since the introduction of EditorTab. The file would be replaced, but not this was not reflected in the editor, which is now fixed. This change allows `Sketch.replaceFile()` to be removed. - When importing a file that does not exist yet (thus adding it), a tab is now also added for it (in addition to a SketchFile). This was broken since the introduction of EditorTab, and would result in the file being added, but not shown in the editor. This commit adds a `Sketch.renameFileTo()` method, to rename a single file within the sketch. It would be better to integrate its contents into `Sketch.renameTo()`, but that does not have access to the `Sketch` instance it is contained in. This will be changed in a future commit.
These methods shouldn't really be in Base (or BaseNoGui, which did the actual work), especially since there is already a `FileUtils.recursiveDelete()` which just does the same thing. This commit removes the code from Base and BaseNoGui and instead uses the method from FileUtils. There is one difference between these methods: the Base methods did not delete files if the "compiler.save_build_files" preference was set. However, the Base methods were only used when deleting a sketch, or deleting an existing folder before overwriting it on save as, so this preference didn't actually do what it was supposed to anyway, so dropping it shouldn't be a problem.
This allows simplifying some other things later.
Now that SketchFile keeps a reference to its Sketch, `SketchFile.renameTo()` can call `Sketch.checkNewFilename()`, so there is no need for the renaming itself to go through Sketch. This changes the parameter for `SketchFile.renameTo()` from File to String, to enforce that only the filename is changed, not the directory name.
This isn't much code, but it makes deletion more consistent with renaming and saving with the SketchController handling the UI part and Sketch actually doing the delete.
The extra "File" in the name was a bit redundant, and this is more consistent with `Sketch.delete()`.
Previously, callers of `SketchFile.delete()` would also call `Sketch.removeFile()`, but letting SketchFile handle this is more robust. This is possible now that SketchFile keeps a reference to Sketch and makes updating the Sketch file list less fragile. Eventually this might be further decoupled by letting SketchFile broadcast a "deleted" event instead.
Previously, the caller of the constructor did this, but now that SketchFile keeps a reference to its containing sketch, it can figure it out itself.
Previously, everywhere where it was needed, the path was requested from BaseNoGui. Because the path is based on a hash of the sketch filename, every caller would get the same path for the same sketch. However, it makes more sense to store the path used for a given sketch inside the Sketch object. This prevents having to pass around or regenerate the build path everywhere, and no longer requires the build path to be deterministic (though it still is in this commit). This allows removing some methods and constructors of which two versions were available - one with a build path argument and one without.
Previously, this used a hash of the sketch filename, so the same build path would be generated for the same sketch between multiple compilations. Now that the build path is stored, this requirement has disappeared, so a random filename can be generated again. While here, this commit also changes the prefix from "build" to "arduino_build_", which makes it a bit more clear what the directory's purpose is.
Comparing a File object automatically takes care of filesystem case sensitivity, whereas strings do not, so this makes the comparison slightly more reliable.
This comment still talked about Processing related stuff which doesn't really apply anymore.
Turns out the approached used to keep caret and scroll position only keeps the scroll position. More code is needed to also keep caret position.
Turns out I dropped a setUndoManager call somewhere, as well typed Redo instead of Undo somewhere.
matthijskooijman
force-pushed
the
editor-cleanup
branch
from
7aa5acc
to
8300081
Compare
|
Rebased and merged |
This pullrequest contains a large number of cleanups and code refactorings concerning the editor part of the IDE, which takes care of loading files, showing an editor and tabs, saving files, modifying files, etc.
The original goal of this PR was to use separate RSyntaxTextArea instances for all tabs, preventing some issues resulting from sharing a single instance, and cleaning up the code around this change. The second goal was to clean up the sketch classes (e.g. Sketch, SketchData, SketchCode and SketchCodeDocument, of which it was not really clear what purpose each served). Now, there is SketchController, Sketch and SketchFile, with better defined purposes.
The commits in this PR jump around the code base a bit, since often one cleanup either required another one to happen before, or allowed one after. Also, some commits are simply small cleanups of things I came across while going through the code. It might be possible to reorder the commits to group related commits, but there are a lot of dependencies between them (both conceptual or just textual), which would make this take a lot of time, which does not seem worth the investment.
Some commits do not seem to make much of an improvement in themselves (or even introduce new ugly code), but these usually prepare for a later cleanup (which often removes the ugly code again).
This PR should mostly leave behavior unchanged, only changing the code. In some cases there are some corner cases which are handled better or differently, and some small things have improved as result of some cleanups as well.
A large part of this code could be further improved and decoupled by applying the Observable pattern, or some other event handling mechanism (e.g. to let the EditorHeader get notified when a file gets renamed, so it can update, instead of having the rename code explicitely refresh the EditorHeader). This is something for a future PR, though, since I haven't yet figured out what, if any, framework would best for doing this easily.
The first two commits of this PR come from #4358, so use that PR for comments about these commits.