Fixing the adjust font restore scroll and adding unit tests #3300
Conversation
| function _refreshAndRestoreScroll() { | ||
| var editor = EditorManager.getCurrentFullEditor(), | ||
| oldHeight = editor.getTextHeight(), | ||
| oldWidth = editor._codeMirror.defaultCharWidth(); |
There was a problem hiding this comment.
This time I didn't add this function as an Editor method, since for now it just used here, but let me know if should just make it a method of Editor.
There was a problem hiding this comment.
I think this is ok here for now since this is the only place scroll is affected. We should consider moving this into Editor whenever this is required from someplace else
ghost
assigned 
|
Assign to myself |
| editor.refreshAll(); | ||
|
|
||
| // Since the height isnt updated after the font-size is changed, we just consider the rendered lines | ||
| // to calulate the new scroll position |
|
@TomMalbran Done with review. Thanks a lot!! Functionality does already look way better than currently is in master! See my comments above to verify if we can tweak it a little bit more. For example, open I may have overlooked something, so let me know what you see. |
|
@jbalsas Thanks for the review, all comments mentioned fixed. I did found the error you mentioned when increasing the font, refreshing the window and restoring the font. It seems that the old height was the same as the new height when that was wrong, so I made CodeMirror calculate the old size before removing the styles and fixed the problem. There is still sometimes a scroll issue after reloading with a not default font-size, but I checked and that isn't coming from here, so it must be another issue coming from #3115. |
| var editor = EditorManager.getCurrentFullEditor(); | ||
| editor._codeMirror.defaultCharWidth(); | ||
| editor.getTextHeight(); | ||
| } |
There was a problem hiding this comment.
Is this block really necessary? It seems to be workarounding something on codemirror, and I actually can't tell the difference between having this calls and not having them. What difference does it make for you?
If the gain is minimal, I'd rather not have this hack here now and get a broken logic when/if it gets fixed in codemirror.
There was a problem hiding this comment.
It is. Not sure if is a CodeMirror issue, or just a timing. Try the following with and without this block:
- Increase the font-size 10 times.
- Reload brackets.
- Restore the font-size
Without this hack on step 3 the scroll jumps several lines, but with it, it works. I checked that the problem was that oldHeight and newHeight had the same value. I am not quite sure what it is happening, but it only happens on the first font adjustment after a reload when the font is not the default size. It will further investigation to figure the problem here.
There was a problem hiding this comment.
Oh, I see now what you mean. However, I don't see any real difference between having the workaround and not having it. I'm testing with current master and cm 6a33096e6021fa5c48abf2867e7b68569168e24a. Could you verify if you're behind any of those?
There was a problem hiding this comment.
I merged with master and I can still see the problem. I added console.log(newHeight, oldHeight) right after the set scroll pos, and checked the numbers in the console and found out that the problem is not only with the restore font size, but with the first font adjustment made after a reload when the font size is not the default size. I'll try to go with a different approach if it can fix this cases without a hack like this.
|
@TomMalbran This looks much better. I think there are still some minor issues with horizontal scroll not being restored correctly, but that shouldn't keep us from merging this. Just a couple of comments before we can wrap this up ;) |
|
@jblas Thanks. What is the horizontal problem you are seeing, I checked and the first word seen never moves more than a few pixels from the rounding, which is what the restore scroll should do. |
|
About the horizontal scrolling (I think it also happens sometimes with vertical), if you are scrolled and reduce the font a lot and then restore it, you'll probably be back to 0. In any case, I don't think those are major issues, and the behaviour looks good enough to me as to merge once we know if we need the workaround or not. We can iron out more details later. btw, although jblas will probably be more than interested in Brackets, maybe he'll sit this one out and let me handle it instead :p |
|
Oh, sorry, not sure how I got it wrong since I usually copy it. I can see the horizontal and vertical problem too, it seems to happen when the font size reaches a size lower than 5 (8 font reductions, font size default is 12). The restore when decreasing the font size isn't great when going down past this size, and seems to be a problem with #3115 again. I checked moving the refresh back as how it was, and makes the reduction work better, but not the restore. I guess it can stay like that and just check if it works better after #3115 is fixed. |
|
Ok, so based on everything... would you say the one we have currently is the overall best behavior we get right now? If so, I'm willing to merge this, but we should keep an eye on #3115 and other codemirror improvements to try to come back and improve this. |
|
For this last issue, yes, but I am working on a better solution for the previous issue, when adjusting the font size after a refresh. |
|
I moved the style injection just before the cm refresh, so now the old font sizes are correctly calculated for every case. |
| * @private | ||
| * Sets the font size and restores the scroll position as best as possible | ||
| * @param {string} fsStr A string with the font size and the size unit | ||
| * @param {string} lhStr A string with the line height and a the size unit |
There was a problem hiding this comment.
May be better to have some long/human-readable/pronounceable param names here? ;)
|
@TomMalbran I think this works quite well all things considered. We'd want to come take another look at some point, but we're good to go for now. Please, check my last comments, basically to document clearly what you've found to be an issue and what'd be worth checking in the future. |
|
@jbalsas All done :) |
|
Looks good! Thanks for your patience ;) Merging! |
Fixing the adjust font restore scroll and adding unit tests
|
Great :) Thank you too for all the reviews and testing! |
As requested I made a new request from #3250 with only the fixes made to the restore scroll and the unit tests. This time the restore scroll works perfect, and I made the unit tests use just one window for all the tests.