-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] PR: Add multiline editing capability in the Editor #4987
base: master
Are you sure you want to change the base?
Conversation
Hello @jnsebgosselin! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-04-29 00:56:30 UTC |
Thanks a lot @jnsebgosselin! This is looking great! @rlaverde, please review this one and give @jnsebgosselin feedback about his implementation. |
@jnsebgosselin great work :-) Question, why did we need to create a new class on top of the base editor? Also, cursors should occupy a bit less than the full height of text so when pile together, they do not look like a single super long line |
@jnsebgosselin Thanks for your contribution, It looks pretty good
I agree with @goanpeca, I think we could use other approach instead of adding a new layout of inheritance to the CodeEditor A solution maybe out of the scope of this PR: something like this:
You could take a look at some pyqode modes https://github.com/pyQode/pyqode.core/blob/master/pyqode/core/modes/autocomplete.py We will need to create a @ccordoba12 @goanpeca What do you think? |
@rlaverde sounds good. But what to do in the meantime :-p |
I don't know, putting more code in the already complex CodeEditor doesn't seem as a solution 😞 |
I really like this idea! @rlaverde, could you implement this so that @jnsebgosselin can use it to implement this feature? |
Which name should I use? Modes? EditorFeatures? any other suggestion? |
This looks neat! I like the way it is possible to easily turn off or on modes with that approach. |
I think |
@jnsebgosselin, please rebase this PR and use @rlaverde's new Editor extensions to implement this feature. |
@jnsebgosselin, @rlaverde can finish this one. Do you agree? |
@ccordoba12 Sure thing. |
@rlaverde, please make this an Editor extension and add tests for it. |
9254e7f
to
7ca3788
Compare
cab35e3
to
114e735
Compare
114e735
to
2951b16
Compare
Oh great, I thought this would be useless, so I'm happy if this helps! I'm sorry I haven't had time to look at this to try to help you further but at this point, I did this so long ago that I don't remember anything lol |
What you did works just fine!, the extension logic on the editor (that we borrowed from I also forgot about this as well, but after reviewing your work again, it all made sense, so thanks again, and no worries :-p |
@jnsebgosselin I must to say I'm impressed, really promising PR/WIP you've created over here! one question, when undoing/redoing... will the multiple selections be preserved? Please take a read to see what I mean by that in this thread https://bugreports.qt.io/browse/QTCREATORBUG-16013 . I'm asking this cos one of the main weak points of Scintilla for instance is the fact that the different selections will be gone when undoing/redoing... But in proper implementations such as SublimeText, Codemirror, Monarch, etc... such selections will be preserved. And just for the record, when I say I'm impressed I really mean it... you won't see this "must to have on a modern text editor" feature on almost any existing Qt widgets out there on all github... so... hats off :) . Pasting some references I've found over time regarding to this subject in case it helps:
Your implementation reminds me to |
Tasks
Pull Request Checklist
modified the
spyder/defaults
directory, or added new icons/assetsDescription of Changes
The idea is to be able to set multiple cursors to edit at different places of the code at the same time like it is done in Sublime for example.
This feature is not supported by the Qt framework, so we need to go around it and re-implement a lot of low level stuff.
Issue(s) Resolved
Fixes #2112
Depends on PR #5002