Fix a regression caused by changing backspace key behavior
ClosedPublic

Authored by safaalfulaij on Sep 2 2017, 5:44 PM.

Details

Summary

This is a patch trying to fix the regression caused by an old patch.
Backspace key behavior in all applications acts by removing the diacritics one by one and then
remove the base character.
For Indic locales this doesn't work, and the whole character should be removed.
This causes a problem in Arabic where we just want to remove the diacritic and
not the whole composed character.

This patch adds a configuration option that is disabled by default. It's purpose
is to switch between the two modes.
Enabling it will use the Indic mode, because the default all software uses is the
normal mode.
For the delete key, the correct way for Arabic is to remove the diacritics as
well, so there is no if statement for it.

Test Plan

KTextEditor compiled normally. Both modes works as expected. No problems reported
about the test while compiling, not sure if it's working right or not.
(I copied my changes from an older version source code (matching my current
configiration). There might be things I missed when doing that.)

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.
safaalfulaij created this revision.Sep 2 2017, 5:44 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 2 2017, 5:44 PM
Restricted Application added subscribers: Frameworks, kwrite-devel. · View Herald Transcript

@hein Are you familiar with this sort of behaviour?

dhaumann added a subscriber: jgrulich.

@jgrulich Can you comment on this, since the old patch that this change refers came from you ? :-) I'm sure you understand this better than me.

  • Add the implementation of backspaceRemoveComposed

The concept with the 'Set' flag is not yet correct.

src/utils/kateconfig.cpp
1241 ↗(On Diff #19368)

Should be: m_backspaceRemoveComposed_Set_(False)

1288 ↗(On Diff #19368)

This should be: m_backspaceRemoveComposed_Set_(false)

2221 ↗(On Diff #19368)

Wrong, correct is: m_backspaceRemoveComposed_Set_

2235 ↗(On Diff #19368)

missing: m_backstapceRemoveComposedSet = true;

src/utils/kateconfig.h
610 ↗(On Diff #19368)

After this line you should add bool m_backspaceRemoveComposed;

644 ↗(On Diff #19368)

You need two flags: one m_backspaceRemoveComposedSet flat that knows whether this option is set for an individual view, and another one called m_backspaceRemoveComposed for the actual config value.
Currently, you only have the one for the actual value.

Just look at how this is e.g. done for m_autoCenterlinesSet and m_autoCenterlines.

  • Fix global and local view values
jgrulich edited edge metadata.Sep 11 2017, 5:06 AM

@jgrulich Can you comment on this, since the old patch that this change refers came from you ? :-) I'm sure you understand this better than me.

Not sure I can say much, I wrote this patch because I got a bug report where for Indic locales they were not able to remove composed characters. while other software (QtCreator, Libreoffice) worked pretty well. Also moving cursor to the left or to the right moved it by one composed character which I assumed should be same behaviour also for backspace/delete so I used same methods to identify one exact character which should be removed.

Not sure I can say much, I wrote this patch because I got a bug report where for Indic locales they were not able to remove composed characters. while other software (QtCreator, Libreoffice) worked pretty well. Also moving cursor to the left or to the right moved it by one composed character which I assumed should be same behaviour also for backspace/delete so I used same methods to identify one exact character which should be removed.

Well, I just checked both QtCreator and LibreOffice (using a text from Wikipedia) and both gave me different results than Kate currently doing. The results are the normal results that Arabic uses.
I used them with thier defaults, so if there is any option hidden somewhere, then it's the same case I'm trying to achieve here.
Where is that bug report?

Not sure I can say much, I wrote this patch because I got a bug report where for Indic locales they were not able to remove composed characters. while other software (QtCreator, Libreoffice) worked pretty well. Also moving cursor to the left or to the right moved it by one composed character which I assumed should be same behaviour also for backspace/delete so I used same methods to identify one exact character which should be removed.

Well, I just checked both QtCreator and LibreOffice (using a text from Wikipedia) and both gave me different results than Kate currently doing. The results are the normal results that Arabic uses.
I used them with thier defaults, so if there is any option hidden somewhere, then it's the same case I'm trying to achieve here.
Where is that bug report?

The bug report is not publicly available, but it was about kate not removing composed characters, while other software do that. I now checked this with LibreOffice and QtCreator and it seems to remove composed character only when you use DELETE and not BACKSPACE. Not sure what is correct, but having this as an option seems to be a good idea.

safaalfulaij added a comment.EditedSep 11 2017, 11:09 AM

The bug report is not publicly available, but it was about kate not removing composed characters, while other software do that. I now checked this with LibreOffice and QtCreator and it seems to remove composed character only when you use DELETE and not BACKSPACE. Not sure what is correct, but having this as an option seems to be a good idea.

Yes that's what is correct, and that's why I didn't add an if statment for changing the behaviour in case you used DELETE; because the “Indic” way is the correct way for Arabic as well. The current patch applies what is used in normal Qt/GTK widgets, both DELETE and BACKSPACE, as the default option. The other option will make BACKSPACE works as DELETE by removing whole composed characters, nothing more.

dhaumann requested changes to this revision.Sep 18 2017, 4:34 PM

I think one further revision would be helpful, then the patch should be good to go in.

src/dialogs/navigationconfigwidget.ui
138 ↗(On Diff #19370)

Typo: their instead of thier.

141 ↗(On Diff #19370)

Typo: its instead of it's

src/document/katedocument.cpp
3170

I would prefer to have this if() only once:

KTextEditor::Cursor beginCursor(line, 0);
KTextEditor::Cursor endCursor(line, col);
if (!view->config()->backspaceRemoveComposed()) { // Normal backspace behavior
    beginCursor.setColumn(col - 1);
    // move to left of surrogate pair
    if (!isValidTextPosition(beginCursor)) {
        Q_ASSERT(col >= 2);
        beginCursor.setColumn(col - 2);
    }
} else {
    beginCursor.setColumn(view->textLayout(c)->previousCursorPosition(c.column()));
}
3189–3195

Same here: Please if() case only once, which leads to less convoluted code.

This revision now requires changes to proceed.Sep 18 2017, 4:34 PM
  • Typos and better if statement structure

All issues addressed, or?

anthonyfieroni added inline comments.
src/document/katedocument.cpp
3169–3212

This peace of code should be simplified, to not have code dublication.

if (config()->backspaceIndents()) {
     // backspace indents: erase to next indent position
      Kate::TextLine textLine = m_buffer->plainLine(line);

      // don't forget this check!!!! really!!!!
      if (!textLine) {
          return;
      }

      int colX = textLine->toVirtualColumn(col, config()->tabWidth());
      int pos = textLine->firstChar();
      if (pos > 0) {
          pos = textLine->toVirtualColumn(pos, config()->tabWidth());
      }

      if (pos < 0 || pos >= (int)colX) {
          // only spaces on left side of cursor
          indent(KTextEditor::Range(line, 0, line, 0), -1);
      }
}

Then validate pos and add 2 backspace behaviors.

@safaalfulaij, any progress on the latest comments?

@safaalfulaij, any progress on the latest comments?

Not really. I was busy with college. I'll try finishing it in the coming week.

  • Applying last comment (hopefully I did what you meant :!)
mwolff resigned from this revision.Nov 19 2017, 9:48 PM
mwolff added a subscriber: mwolff.

ping? can this go in?

If I don't misread the current diff, all things got done.
Can you commit or shall I do so for you?

cullmann accepted this revision.Nov 21 2017, 7:57 PM

I can commit but it is showing as not accepted.
So I'll just wait for the final confirmation.

This revision was automatically updated to reflect the committed changes.
brauch added a subscriber: brauch.Dec 23 2017, 10:40 PM

After this was submitted master doesn't compile for me, and if I fix the compile in the trivial way the test fails. Can you have another look?

brauch reopened this revision.Dec 23 2017, 10:44 PM
brauch added inline comments.
autotests/src/katedocument_test.cpp
452

must be view->config ... how did this patch ever compile for you??

src/document/katedocument.cpp
3191

pos is not defined at this place

Fix compilation issues

Can the compile fix please be committed?
Otherwise we need to revert the entire patch to ensure Frameworks remains buildable.

This revision was automatically updated to reflect the committed changes.