D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko added a comment.


  In D28819#651007 , @dfaure wrote:
  
  > The *ideal* way of proceeding is to submit a Qt bug report with your 
testcase (after searching for an existing Qt bug report for the same issue), 
then turn it into a Qt autotest and make a gerrit merge request for Qt with 
both the test and a fix for it. And then, and only then, add a workaround in 
krichtextedit with a Qt version ifdef and a reference to the Qt bug report.
  >
  > The less ideal way is to only do the Qt bug report and the workaround 
here...
  
  
  Thanks for your comment! 
  I already did the first part (that is, Qt bugreport 
).
  I can play around Qt code too, and see if I can prepare a test & patch.
  Although, to be honest, I'm a bit afraid to touch it. Most likely there was 
some reasoning behind this "dirty" value, which I can't yet guess by looking at 
the code (it lacks documentation / comments) :(

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[David Faure]

dfaure added a comment.


  The *ideal* way of proceeding is to submit a Qt bug report with your testcase 
(after searching for an existing Qt bug report for the same issue), then turn 
it into a Qt autotest and make a gerrit merge request for Qt with both the test 
and a fix for it. And then, and only then, add a workaround in krichtextedit 
with a Qt version ifdef and a reference to the Qt bug report.
  
  The less ideal way is to only do the Qt bug report and the workaround here...

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko added a comment.


  @dfaure There seem to be a regression caused by this patch: if I reach the 
end of current view, and then press Enter so it has to scroll down, it scrolls 
to the very beginning of the document instead.
  
  This is the same issue as in ancient bug 195828 
, and it seems like it is Qt issue.
  
  Here's the simplest MWE:
  
#include 
#include 
#include 
#include 
#include 

int main(int argc, char *argv[])
{
QApplication a(argc, argv);
auto w = new QWidget();
auto edit = new QTextEdit();
auto button = new QPushButton(QStringLiteral("Insert new line"));
button->connect(button, ::clicked, [edit](){
auto cursor = edit->textCursor();
cursor.beginEditBlock();
cursor.insertBlock();
edit->setTextCursor(cursor);
cursor.endEditBlock();
});
auto layout = new QVBoxLayout(w);
layout->addWidget(edit);
layout->addWidget(button);
w->show();
return a.exec();
}
  
  Seems like the following code in `QTextCursorPrivate::setX` causes the 
trouble:
  
if (priv->isInEditBlock() || priv->inContentsChange) {
x = -1; // mark dirty
return;
}
  
  and seems like this "dirty" value is treated literally afterwards.
  
  For this example, switching `endEditBlock` and `setTextCursor` lines fixes 
the issue (and same with `Insert Rule`).
  I've also found a (dirty) workaround for the original patch: if I call 
`setTextCursor(textCursor())` right after `endEditBlock()`, it works like a 
charm. It seems like this line (although semantically is a NOOP) triggers the 
update of this "dirty" value.
  
  How should I proceed?

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R310:1d1eb6feb7f8: [KRichTextEdit] Always treat key press as 
single modification in undo stack (authored by poboiko).

REPOSITORY
  R310 KTextWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28819?vs=80084=80164

REVISION DETAIL
  https://phabricator.kde.org/D28819

AFFECTED FILES
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R310 KTextWidgets

BRANCH
  single-undo (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Ahmad Samir]

ahmadsamir added a reviewer: dfaure.
ahmadsamir added a comment.


  I tested this with the krichtexteditor test app from kxmlgui, and it works. 
(It's never too late to fix a bug, even if it's from 2010 :)).

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R310 KTextWidgets

REVISION DETAIL
  https://phabricator.kde.org/D28819

To: poboiko, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28819: [KRichTextEdit] Always treat key press as single modification in undo stack

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Frameworks, mlaurent.
Herald added a project: Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  If my cursor is on a bullet list, and I press Enter to create a new element,
  it registers as 5-7 actions in an undo stack, mostly margins adjustments. So 
to
  rewind it, user has to press `Ctrl+Z` 5+ times, which is quite frustrating.
  
  Instead this patch suggests to treat _any_ change that is a result of a single
  key press as a single event, so user has to press `Ctrl+Z` just once to undo 
it

TEST PLAN
  1. Open KMail -> New Mail, enable Rich Text mode (or, say, open KJots)
  2. Create a bullet list with couple of elements
  3. Put cursor on the last element, press Enter to create a new element
  4. Try to undo this action with Ctrl+Z
  5. (without patch) One has to press Ctrl+Z 5-7 times to undo it.
  6. (with patch) A single Ctrl+Z is enough

REPOSITORY
  R310 KTextWidgets

BRANCH
  single-undo (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D28819

AFFECTED FILES
  src/widgets/krichtextedit.cpp

To: poboiko, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns