This revision was automatically updated to reflect the committed changes.
Closed by commit R127:59259d8dc1f3: Add XDG WM Base support to our XDGShell API
(authored by davidedmundson).
REPOSITORY
R127 KWayland
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D13510?vs=36244=37872
romangg accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R127 KWayland
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D13510
To: davidedmundson, #kwin, romangg
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns
davidedmundson added a comment.
> Or would it make sense to wait on merging until Qt 5.12 with XDG WM Base
support is available to have more test candidates?
I don't think it's needed. The protocol is 90% the same it's mostly just
awkward name changes. Same on the Qt side.
We
romangg added a comment.
From my side I can't see anything wrong right now. Since we would merge after
the next release anyways we have some time to correct any overlooked mistakes
afterwards. Or would it make sense to wait on merging until Qt 5.12 with XDG WM
Base support is available to
davidedmundson added a dependent revision: D13530: Add XDG WmBase support.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D13510
To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns
davidedmundson removed a dependency: D13530: Add XDG WmBase support.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D13510
To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns
davidedmundson added a dependency: D13530: Add XDG WmBase support.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D13510
To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns
davidedmundson added a comment.
> XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED
I'm not sure. There's an implication that XDG_SURFACE errors are meant for in
response to requests on an xdg_surface interface, (i.e for use with
xdg_surface.get_toplevel xdg_surface.get_popup) whereas this is a
davidedmundson updated this revision to Diff 36244.
davidedmundson added a comment.
use static_cast in client
REPOSITORY
R127 KWayland
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D13510?vs=36206=36244
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D13510
zzag added inline comments.
INLINE COMMENTS
> xdgshell_stable.cpp:305
> +Q_UNUSED(surface)
> +auto s = reinterpret_cast(data);
> +s->q->configureRequested(s->pendingSize, s->pendingState, serial);
Did you forget to change it to static_cast? Or there is a reason why it's still
romangg added inline comments.
INLINE COMMENTS
> davidedmundson wrote in xdgshell_stable_interface.cpp:246
> What role do you think it should be?
>
> I'm using the error for when a wl_surface has another role in the case that
> you call get_xdg_surface twice on the same wl_surface, which seems
davidedmundson updated this revision to Diff 36206.
davidedmundson added a comment.
Roman's comments (except the one I commented on)
REPOSITORY
R127 KWayland
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D13510?vs=36127=36206
BRANCH
master
REVISION DETAIL
davidedmundson marked 4 inline comments as done.
davidedmundson added inline comments.
INLINE COMMENTS
> romangg wrote in xdgshell_stable_interface.cpp:246
> Wrong id `XDG_WM_BASE_ERROR_ROLE`.
What role do you think it should be?
I'm using the error for when a wl_surface has another role in
romangg added inline comments.
INLINE COMMENTS
> registry.cpp:88
> #include
> +#include
>
nitpick: put this include directly below the other xdg-shell ones above.
> xdgshell_stable.cpp:195
> +
> +uint32_t constraint = 0;
> +if
>
davidedmundson updated this revision to Diff 36127.
davidedmundson marked 2 inline comments as done.
davidedmundson added a comment.
ZZag's comments
REPOSITORY
R127 KWayland
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D13510?vs=36089=36127
BRANCH
master
REVISION DETAIL
davidedmundson marked 10 inline comments as done.
davidedmundson added inline comments.
INLINE COMMENTS
> zzag wrote in test_xdg_shell.cpp:120
> Missing `default`. Is it okay if version is unknown?
IMHO it's better not to as it means we get a compile time warning in the
unlikely event that
zzag added inline comments.
INLINE COMMENTS
> zzag wrote in xdgshell_stable.cpp:525
> Shouldn't it be also static?
Ignore it.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D13510
To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns
zzag added inline comments.
INLINE COMMENTS
> test_xdg_shell.cpp:120
> +break;
> +}
>
Missing `default`. Is it okay if version is unknown?
> xdgshell_stable.cpp:28
> +
> +#include
> +
Seems like QDebug is not used here.
> xdgshell_stable.cpp:307
> +Q_UNUSED(surface)
> +
davidedmundson created this revision.
davidedmundson added a reviewer: KWin.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
davidedmundson requested review of this revision.
REVISION SUMMARY
This adds XDG WM Base (essentially
19 matches
Mail list logo
