Add option to treat some chars also as "auto bracket" only when we have a selection
ClosedPublic

Authored by loh.tar on Dec 19 2018, 4:58 PM.

Details

Summary

With that option you can add e.g. Markdown markups, or what ever else may handy,
afterwards to some text like you can do with quotes and enabled auto brackets.

The combobox option for that feature offer a few suggestions which you can
modify as you like which will add a new char set. Clear the set, e.g. by the
line edit button, will remove the set from the combobox. The default suggestions
can't be removed.

Issues

  • Add/Remove a set works seemingly but when click on the "show list button" of

the box immediately after such change is the popup not perfectly updated

Test Plan

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
loh.tar created this revision.Dec 19 2018, 4:58 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptDec 19 2018, 4:58 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Dec 19 2018, 4:58 PM

Should you dislike the auto bracket code changes can I remove them from this patch. They are not needed for this one and only be the byproduct of my code studying.

As always, poor tested :-)

sars added a subscriber: sars.Dec 20 2018, 8:07 AM
sars added inline comments.
src/document/katedocument.cpp
117

I'm not familiar with the auto-bracket code, but the change from QChar to const QChar & does not improve the amount of copied data.

Qt documentation: "Most compilers treat it like an unsigned short."

This means that in stead of adding unsigned short to the stack you add the 64bit reference and add the tiny overhead that the reference brings with it.

It probably has no practical difference tho....

the change from QChar to const QChar & does not improve the amount of copied data.

Argh! OK. Was just an impulse to do it as usual and to follow isStartBracket(..) and isBracket(..)
Will wait for further advice

Btw., have you perhaps played a bit with the https://phabricator.kde.org/D12295 patch?
I tried it once and it did behave ok, but I don'T use that feature that often.
If you use it, could you try that patch, too?

Btw., have you perhaps played a bit with the https://phabricator.kde.org/D12295 patch?

No, but noticed it sometime in the past

If you use it, could you try that patch, too?

Will try it

Will wait for further advice

Still waiting if I have to undo these reference stuff. (Yes your are busy. Just only wanted it to emphasize)
Besides, I like to add some more chars to this special handling:

_ : # @ ~ * ! $ % / \

Some of these may handy when write Markdown, some perhaps elsewhere

loh.tar updated this revision to Diff 49452.Jan 14 2019, 3:50 PM
loh.tar retitled this revision from DocumentPrivate: Treat angle bracket < and backtick ` also as "auto bracket" when we have a selection to DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection.
loh.tar edited the summary of this revision. (Show Details)
  • Add chars ´ ` _ . : | # @ ~ * ! ? $ % / \ = to special handling
  • Avoid chars[0]
cullmann requested changes to this revision.Jan 20 2019, 12:25 PM

Played a bit with it, works reasonable.
Just replace const QChar &c with const QChar and we let this in.

This revision now requires changes to proceed.Jan 20 2019, 12:25 PM
loh.tar updated this revision to Diff 50061.Jan 22 2019, 2:26 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • const QChar as value, not reference
  • Add , ; - + ^ ° § & just for completeness
mwolff added a subscriber: mwolff.Jan 23 2019, 8:21 PM

can you say what this exactly does? from reading the code and the somewhat vague commit message, it makes me believe that when I have anything selected and then press e.g. ? the selection becomes wrapped in two ? - is that right? when would we ever want this?

and if we only want this for, say, markdown, then it should be a per-highlightfile list of chars that trigger this special behavior. In C++ e.g. I would hate if I couldn't just select code and replace it by an operator anymore, e.g. if I have foo + bar and I select the + bar part and then start typing - asdf, would I now suddenly get foo -+ bar- asdf?!

the selection becomes wrapped in two ? - is that right?

Yes

if we only want this for, say, markdown, then it should be a per-highlightfile list of chars that trigger this special behavior

How that?

if I have foo + bar and I select the + bar part and then start typing - asdf, would I now suddenly get foo -+ bar- asdf?!

Just try it. Almost right. Due it's behavior to move the cursor at the start of the selection and to keep the selection it becomes foo - asfd-

In C++ e.g. I would hate if I couldn't just select code and replace it by an operator anymore,

Hm, I see. IIRC has the highlight stuff functions to ask the type of something. So probably there is a isOperator(QChar c) function available. Will then try that.

If not. Would it be too annoying to type - SPACE asdf DEL?

There is an enum KSyntaxHighlighting::Theme::TextStyle::Operator but I can't find a function to test for that.
Beside that I guess it would not work anyway. If it's an operator or not need a call to some "update highlight" to investigate the context.

Ideas?

Either one would need to expose more info from the xml highlighting definitions or as poor mans fix provide some config entry to allow people themself to specify which () "" .... pairs the want to have handled globall with minimal defaults.

mwolff requested changes to this revision.Jan 24 2019, 3:34 PM

Ok, that behavior you describe is bad. I would get annoyed by that, esp. since the list of chars that you use are totally language specific.

If you want to get this behavior for Markdown, then we need to store that data in the syntax file and make it accessible at this position. in the <general> section we already define how single/multiline comments are handled. We would need something similar for this feature, e.g. add something like the following and make it accessible through the syntax-highlighting API:

<general>
  <brackets>
    <bracket open="(" close=")" />
    <bracket open="[" close="]" />
    <bracket open="{" close="}" />
    <bracket open="&lt;" close="&gt;" />
    <bracket open="&quot;" close="&quot;" />
    <bracket open="'" close="'" />
    <bracket open="´" close="´" />
    <bracket open="`" close="`" />
    <bracket open="_" close="_" />
    <bracket open="." close="." />
    <bracket open="," close="," />
    <bracket open=";" close=";" />
    <bracket open=":" close=":" />
    <bracket open="|" close="|" />
    <bracket open="-" close="-" />
    <bracket open="+" close="+" />
    <bracket open="^" close="^" />
    <bracket open="°" close="°" />
    <bracket open="&amp;" close="&amp;" />
    <bracket open="#" close="#" />
    <bracket open="@" close="@" />
    <bracket open="~" close="~" />
    <bracket open="*" close="*" />
    <bracket open="!" close="!" />
    <bracket open="?" close="?" />
    <bracket open="$" close="$" />
    <bracket open="§" close="§" />
    <bracket open="%" close="%" />
    <bracket open="/" close="/" />
    <bracket open="\" close="\" />
    <bracket open="=" close="=" />
  </brackets>
</general>

Of course the initial, common, brackets should be the implicit default when no <brackets> group is specified in a language file. And then we probably need more meta data that specified that some of the brackets above only should be added when we have a selection. Just add a requiresSelection or similar attribute?

This revision now requires changes to proceed.Jan 24 2019, 3:34 PM

If you want to get this behavior for Markdown

Even I had mentioned Markdown as example, I would prefer to have this as general behavior. Not only because I don't know anything about that highlight stuff and can't implement it this way.

I often write some text in "Normal" mode and may miss then that. I tend to add some option for that as @cullmann suggested. BTW he was not annoyed while testing it (well, with an earlier version) So it may less annoying than thought. I can remember I was also sometimes annoyed by the normal auto-bracket feature, it may only need some familiarization.

loh.tar updated this revision to Diff 57125.Apr 28 2019, 11:14 AM
loh.tar retitled this revision from DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection to Add option to treat some chars also as "auto bracket" only when we have a selection.
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
loh.tar added a subscriber: ngraham.
  • Make feature optional by new config interface
  • Make feature independent from auto bracket option
  • Remove redundant const QChar typedChar
  • Use dummy config value from dialog

TODO

  • Add QLineEdit or similar to config dialog
  • Oops, remove/change special angle bracket stuff

QUESTIONS

  • Would it be this way in general OK? @mwolff
  • Is the chosen option name fitting and descriptive enough? @ngraham
loh.tar updated this revision to Diff 57192.Apr 29 2019, 3:03 PM
loh.tar edited the test plan for this revision. (Show Details)
  • Remove special angle bracket check, will be back in a smarter may later (hopefully)
  • Move getter config functions into header
cullmann requested changes to this revision.Apr 30 2019, 6:05 PM

I think this would be some initial solution for this issue. An highlighting specific config would be even cooler, but giving the user the choice to add additional chars (or alter the wanted ones) is definitely a step in the right direction.
Therefore I would say: proceed with this.
I think it would be best to have as default value here the current chars that we use to allow people even to remove some they dislike.
With the new config entry framework we can even expose this as mode-line thing later.

This revision now requires changes to proceed.Apr 30 2019, 6:05 PM
loh.tar updated this revision to Diff 57733.May 7 2019, 7:09 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
loh.tar added a reviewer: VDG.
  • Make feature configurable

I am not sure about the "UserSetsOfCharsToEncloseSelection" part.
Wouldn't it be enough to have the "CharsToEncloseSelection" config part and have the other thing just internal in the editor config page as "template" the user can select to fill the selection of characters?
I don't see the real benefit to have this as config.

cullmann requested changes to this revision.May 8 2019, 5:39 PM
This revision now requires changes to proceed.May 8 2019, 5:39 PM

Well, I'm also not full convinced about the current way, but how else?

The point is, you can now add as much sets as you like and rel. quickly change between your presets. I read you request so, that we have at the end only some hardcoded offers which the user can modify, but saved is only one setting. Not so user-friendly :-/

When one setting always fit, why at all make it configurable?

I think I misunderstood the feature, sorry.

I did now try the patch again (it didn't apply perfectly, but that was just a few lines of code to be moved in the katedocument.cpp patch. I tend to agree that until we have perhaps some proper config for that in syntax files, some presets and the ability for users to add own presets make sense.

On can still later introduce some "syntax defined" enum value to query stuff from the syntax defintions.

What I would change is the placement of the SetOfCharsToEncloseSelection enum, I think it is only needed in the dialog, or? I got partly confused by that as I thought it would be used to store the settings, but the settings are just stored as stringlist and the enum is only to make the UI aware of the presets, or?

cullmann accepted this revision.Jul 13 2019, 4:26 PM

I think the latest state here is usable.
I played more with it, and it behaves well (and is default off).
Before the patch applies even less than now, I will merge this.
Thanks for the addition.

This revision was not accepted when it landed; it landed in state Needs Revision.Jul 13 2019, 4:31 PM
This revision was automatically updated to reflect the committed changes.