Fix captions with non-BMP characters
ClosedPublic

Authored by cfeck on Feb 15 2019, 8:23 PM.

Details

Summary

KWin replaces any non-printable character with a space. This check does not handle surrogate pairs correctly. Additionally, translators sometimes insert non-printable soft-hyphens into titles, which also cause KWin to display a space instead.

This code adds the missing surrogate handling, and (to fix both issues), also removes non-printable characters instead of replacing them with a space.

Also moved the changed test after these changes, so that changes in non-printable characters do not cause unneeded redraws.

BUG: 376813
FIXED-IN: 5.15.3

Test Plan

kwrite /tmp/Test😣.txt shows correct title. I also tested actual non-printable characters, such as 0x1A, and these are correctly omitted.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
cfeck created this revision.Feb 15 2019, 8:23 PM
Restricted Application added a project: KWin. Β· View Herald TranscriptFeb 15 2019, 8:23 PM
Restricted Application added a subscriber: kwin. Β· View Herald Transcript
cfeck requested review of this revision.Feb 15 2019, 8:23 PM

I think we have a unit test for it which should now expect pass. If I'm wrong and we don't have a test case: could you please add one.

cfeck added a comment.Feb 16 2019, 5:38 PM

I would need guidance to write such a test. If I understand it, I would need to create a dummy Client subclass, call setCaption() with different strings, and check what actually lands in the cap_normal variable.

The question is, how can I create a such a dummy Client when it actually requires a workspace? Any tests that do similar things with a different property?

I would need guidance to write such a test. If I understand it, I would need to create a dummy Client subclass, call setCaption() with different strings, and check what actually lands in the cap_normal variable.

The question is, how can I create a such a dummy Client when it actually requires a workspace? Any tests that do similar things with a different property?

Have a look at void X11ClientTest::testCaptionSimplified() in autotests/integration/x11_client_test.cpp - it's a very similar case.

cfeck updated this revision to Diff 53362.Mar 7 2019, 1:56 PM
cfeck edited the summary of this revision. (Show Details)

Added a test case. I couldn't find any mention where the testCaptionSimplified() is referenced, so I assume the new test testCaptionNonPrintable() is also picked-up automatically.

I couldn't run the autotests because my system does not have Wayland.

cfeck updated this revision to Diff 53364.Mar 7 2019, 1:59 PM
zzag added a subscriber: zzag.Mar 8 2019, 8:12 AM
zzag added inline comments.
autotests/integration/x11_client_test.cpp
154–155 β†—(On Diff #53364)

Do we need these?

197–198 β†—(On Diff #53364)

Can it be moved up?

cfeck added a comment.Mar 8 2019, 12:20 PM

Note that the added test is a verbatim copy (except for the changed text) of testCaptionSimplified(), so Martin could answer these questions. If changes made here make sense, they also could be made to the other test.

zzag added a comment.EditedMar 8 2019, 12:53 PM

If changes made here make sense, they also could be made to the other test.

Yep, we could unify them by using a test table, e.g.

void X11ClientTest::testCaption_data()
{
    QTest::addColumn<QByteArray>("original");
    QTest::addColumn<QByteArray>("expected):

   QTest::newRow("trimmed") << QByteArrayLiteral("...") << QByteArrayLiteral("...");
   QTest::newRow("emojis") << QByteArrayLiteral("...") << QByteArrayLiteral("...");
}

void X11ClientTest::testCaption
{
    // create an xcb window
    QScopedPointer<xcb_connection_t, XcbConnectionDeleter> c(xcb_connect(nullptr, nullptr));
    QVERIFY(!xcb_connection_has_error(c.data()));
    const QRect windowGeometry(0, 0, 100, 200);
    xcb_window_t w = xcb_generate_id(c.data());
    xcb_create_window(c.data(), XCB_COPY_FROM_PARENT, w, rootWindow(),
                      windowGeometry.x(),
                      windowGeometry.y(),
                      windowGeometry.width(),
                      windowGeometry.height(),
                      0, XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0, nullptr);
    xcb_size_hints_t hints;
    memset(&hints, 0, sizeof(hints));
    xcb_icccm_size_hints_set_position(&hints, 1, windowGeometry.x(), windowGeometry.y());
    xcb_icccm_size_hints_set_size(&hints, 1, windowGeometry.width(), windowGeometry.height());
    xcb_icccm_set_wm_normal_hints(c.data(), w, &hints);
    NETWinInfo winInfo(c.data(), w, rootWindow(), NET::Properties(), NET::Properties2());
    QFETCH(QByteArray, original);
    winInfo.setName(original);
    xcb_map_window(c.data(), w);
    xcb_flush(c.data());

    // we should get a client for it
    QSignalSpy windowCreatedSpy(workspace(), &Workspace::clientAdded);
    QVERIFY(windowCreatedSpy.isValid());
    QVERIFY(windowCreatedSpy.wait());
    Client *client = windowCreatedSpy.first().first().value<Client*>();
    QVERIFY(client);
    QCOMPARE(client->window(), w);
    QFETCH(QByteArray, expected);
    QCOMPARE(client->caption(), QString::fromUtf8(expected));

    // and destroy the window again
    xcb_unmap_window(c.data(), w);
    xcb_destroy_window(c.data(), w);
    xcb_flush(c.data());

    QVERIFY(Test::waitForWindowDestroyed(client));
}
zzag added a comment.Apr 25 2019, 11:27 PM

What's the status of this patch?

cfeck added a comment.Apr 26 2019, 6:13 AM

As indicated above, I cannot actually run the tests. I could blindly try the suggestion you made, if someone else verifies they still work.

zzag added a comment.Apr 26 2019, 7:15 AM

Would you mind if I temporarily commandeer this revision and implement the tests?

cfeck added a comment.Apr 26 2019, 7:24 AM

No, go ahead :)

zzag commandeered this revision.Apr 26 2019, 7:52 AM
zzag added a reviewer: cfeck.
zzag updated this revision to Diff 57007.Apr 26 2019, 7:53 AM

Tests.

zzag added a comment.Apr 26 2019, 7:55 AM

@cfeck Please commandeer this revision.

cfeck commandeered this revision.Apr 26 2019, 8:25 AM
cfeck edited reviewers, added: zzag; removed: cfeck.

Thanks!

zzag accepted this revision.Apr 26 2019, 8:36 AM
zzag added inline comments.
client.cpp
1484–1485

Missing braces.

This revision is now accepted and ready to land.Apr 26 2019, 8:36 AM
This revision was automatically updated to reflect the committed changes.