Community
Participate
Working Groups
I20070821-0800 When enabling format on save the high amount of outgoing changes due to formating changes is a major annoyance/makes it unusable for me. The ability to format (and maybe other actions) only changed regions would be a substantial improvement for this feature. I've wrote a prototype which does use the LastSaveReferenceProvider to determine the change region between the current and last save. I'll attach a patch. This enhancement has another nice side effect: Sometimes the formatter does format code to a form one does not want. Currently with format on save there is no way out, another major annoyance. With this patch all you do is 'undo' save action and format on save will not change the formatting next time you save (unless you change the region again of course).
Created attachment 76890 [details] prototype This prototype is more a sketch of what the implementation could like like then a final fix. I guess the main concern here is performance. Also other clean ups must be able to work only on changed regions. Of course the other solution for my problem would be to format jdt/ui according to the formatter profile...
>(and maybe other actions) Yes, it would be strange if formatting is restricted to changed code but cleanup is done on the whole file. The main problem is that the LastSaveReferenceProvider adds yet another copy of the whole document. This is not so good as we already try to fight that (see bug 75555 and bug 75086). Workaround is to clean and format the project and then use the Save As feature.
Created attachment 77590 [details] proposed fix This fix does not require to store a copy of the buffer: A compare between the version on disk and the current version is made before committing the working copy. An option is added to the format save action: 'Only format changed regions (experimental)' Of course the disk access and the compare has a price, but I'm using it all day long and have had no problems.
>Of course the disk access and the compare has a price, but I'm using it all day >long and have had no problems. Did you test with large files with many changes? A quick test using StyledText revealed that it takes > 2s to only compute the changes (thousands!) and very long to notify the post saved listener. Such a worst case scenario can easily happen if the user formats the code himself before saving the file. Sorry but I don't want yet more file read and BOM code duplication. We'd need to find another way to do this. Also, what if the user changed the encoding but didn't save yet? In this case you would read the file with the wrong (new) encoding. The new code adds danger (while computing the diff) as it does work before the file gets saved to disk. postSaveListenersRequireChangedRegions(...) should read like is* does etc. and it would be better moved to the SaveParticipantRegistry.
We could avoid the file read code duplication by allowing clients to instantiate their own file buffer manager.
Created attachment 78353 [details] proposed fix (In reply to comment #4) > Did you test with large files with many changes? A quick test using StyledText > revealed that it takes > 2s to only compute the changes (thousands!) and very > long to notify the post saved listener. Such a worst case scenario can easily > happen if the user formats the code himself before saving the file. Changed regions are now line based, this reduces the amount of changed regions dramatically in your worst case scenario which increases the performance substantially. The part which takes very long is explained in Bug 203304 and needs to be resolved. With bug 203304 it takes only a few seconds to save in a worst case scenario (AbstractTextEditor with many changes), I think this is acceptable for this corner case. > Sorry but I don't want yet more file read and BOM code duplication. We'd need > to find another way to do this. Also, what if the user changed the encoding but > didn't save yet? In this case you would read the file with the wrong (new) > encoding. Ok, using (internal) reference to TextFileBufferManger to load the file. > The new code adds danger (while computing the diff) as it does work before the > file gets saved to disk. Ok, using safe runners, commit the working copy anyway if something went wrong. However, I'm not sure if fireElementStateChangeFailed needs to be called in this case. > postSaveListenersRequireChangedRegions(...) should read like is* does etc. and > it would be better moved to the SaveParticipantRegistry. changed the name, did not move because the exception handle would be too complicated.
>Ok, using (internal) reference to TextFileBufferManger to load the file. Please add API to FileBuffers: getPrivateTextFileBufferManager() which returns a new one each time.
Created attachment 78432 [details] fix Updated patch, even with tests! ;-)
Created attachment 79294 [details] fix Adopted patch to latest head code.
Created attachment 79390 [details] fix Next try... This requires the core formatter patch.
Created attachment 79393 [details] fix None conflicting patch without core requirement Changes in CodeFormatFix and CodeFormatterUtil
Revised version of last patch committed to HEAD together with Benno.
