DocumentPrivate: Treat some chars also as "auto bracket" only when we have a selection
Needs RevisionPublic

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

Details

Reviewers
cullmann
mwolff
Group Reviewers
KTextEditor
Summary

My first attempt was only to add angle brackets to the auto bracket code but
that caused surprising odd behavior in the normal edit flow, so it needs to be
done in a special way which obviously cause no trouble.

Once done this way, I had the idea to add much more char to this special
handling. Some of these may handy when write Markdown, some perhaps elsewhere.

  • Improve readability of auto bracket code
  • Harmonize QChar param as const value, not reference
  • Remove unneeded code
  • Avoid chars[0]

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
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.