This revision was automatically updated to reflect the committed changes.
Closed by commit R108:c87230c3a5fe: Use xcb-icccm to read the name property
(authored by graesslin).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D7046?vs=17587=18496#toc
REPOSITORY
R108 KWin
CHANGES SINCE
fvogt accepted this revision.
fvogt added a comment.
This revision is now accepted and ready to land.
Nobody seems to be against it and the code looks good to me + it works fine
here. So I don't see a reason to let this laying around for longer.
REPOSITORY
R108 KWin
BRANCH
xcb-iccm-name
graesslin added a comment.
ping
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D7046
To: graesslin, #kwin, #plasma
Cc: cfeck, anthonyfieroni, fvogt, broulik, plasma-devel, kwin, ZrenBot,
progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas,
cfeck added inline comments.
INLINE COMMENTS
> graesslin wrote in client.cpp:1428
> Just to add a little bit of context here: we are discussing two memcopies
> here in a code path which performs a roundtrip to the X server. This thing is
> going to be slow and the memcopy does not matter at
graesslin added inline comments.
INLINE COMMENTS
> cfeck wrote in client.cpp:1428
> If Qt6 would introduce UTF-8 encoded QString (so that QString::fromUtf8()
> would no longer need to create a new array), then you can expect all Qt
> software to break. QString::fromUtf8() is used nearly
cfeck added inline comments.
INLINE COMMENTS
> graesslin wrote in client.cpp:1428
> I'll keep the second copy nevertheless. After all this mess with Qt 5
> transition I don't trust Qt in the area of QString from char array.
>
> I rather have here a copy too much, which won't matter, then a
fvogt added a comment.
In https://phabricator.kde.org/D7046#133383, @graesslin wrote:
> ping
Still works fine here, but I can't really comment on the sanity of the code
changes.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D7046
To: graesslin, #kwin,
graesslin added a comment.
ping
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D7046
To: graesslin, #kwin, #plasma
Cc: anthonyfieroni, fvogt, broulik, plasma-devel, kwin, ZrenBot, progwolff,
lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol,
graesslin updated this revision to Diff 17587.
graesslin added a comment.
Restricted Application edited projects, added KWin; removed Plasma.
Same for fetchIconicName
REPOSITORY
R108 KWin
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7046?vs=17573=17587
BRANCH
xcb-iccm-name
graesslin added inline comments.
INLINE COMMENTS
> fvogt wrote in client.cpp:1428
> That does not matter in this instance.
> `QString::fromUtf8(QByteArray(reply.name, reply.name_len));` does two copies:
>
> - QByteArray copies name_len bytes from name into a heap-allocated buffer
> - QString
graesslin updated this revision to Diff 17573.
graesslin added a comment.
Restricted Application edited projects, added Plasma; removed KWin.
Add simplified()
REPOSITORY
R108 KWin
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7046?vs=17515=17573
BRANCH
xcb-iccm-name
REVISION
fvogt added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in client.cpp:1428
> QByteArray has a move constructor, it's not copy.
That does not matter in this instance.
`QString::fromUtf8(QByteArray(reply.name, reply.name_len));` does two copies:
- QByteArray copies name_len bytes
anthonyfieroni added inline comments.
INLINE COMMENTS
> fvogt wrote in client.cpp:1428
> Why not directly `QString::fromUtf8(reply.name, reply.name_len)` here? It
> would save one unnecessary copy. Alternatively,
> `QByteArray::fromRawData(reply.name, reply.name_len)`, but that's more
>
fvogt added inline comments.
INLINE COMMENTS
> graesslin wrote in client.cpp:1428
> fromRawData is dangerous as we delete the data with the wipe call again.
> That's why I didn't use it. QString::fromUtf8 is something I didn't use as
> QString from ascii is discouraged and can result in
graesslin added inline comments.
INLINE COMMENTS
> fvogt wrote in client.cpp:1428
> Why not directly `QString::fromUtf8(reply.name, reply.name_len)` here? It
> would save one unnecessary copy. Alternatively,
> `QByteArray::fromRawData(reply.name, reply.name_len)`, but that's more
> verbose.
fvogt added a comment.
Built and tested - works fine! I only added two rather nitpicky comments.
INLINE COMMENTS
> client.cpp:1428
> +if (reply.encoding == atoms->utf8_string) {
> +retVal = QString::fromUtf8(QByteArray(reply.name,
> reply.name_len));
> +
broulik added inline comments.
INLINE COMMENTS
> client.cpp:1433
> +xcb_icccm_get_text_property_reply_wipe();
> +return retVal;
> +}
Missing a `simplified()`
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D7046
To: graesslin, #kwin,
graesslin created this revision.
Restricted Application added a project: KWin.
Restricted Application added subscribers: kwin, plasma-devel.
REVISION SUMMARY
The KWindowSystem call which we used doesn't work on Wayland as it's only
implemented in the xcb variant and cannot be made available
18 matches
Mail list logo
