D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson abandoned this revision. davidedmundson added a comment. All these problems go away with KWaylandServer \o/ REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
zzag added inline comments. INLINE COMMENTS > display.h:281-288 > /** > * Creates the XdgOutputManagerInterface > * > * @return the created manager > * @since 5.47 > + * @deprecated use the version that takes a version > */ Please use KWAYLAND_ENABLE_DEPRECATED_SINCE to make the compiler bark whenever someone uses the deprecated version of createXdgOutputManager. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson added inline comments. INLINE COMMENTS > zzag wrote in display.h:297 > You can't introduce another createXdgOutputManager() because it's not > overloaded. You probably need to rename this method, e.g. > createXdgOutputManager2. As you've pasted, it is BC, which is what frameworks promises. It's something that we definitely do currently. It's also SC for literally every conceivable invocation, and we know how the only user of this class is using it. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
zzag added inline comments. INLINE COMMENTS > apol wrote in display.h:296 > Passing an enum as const& is wrong although it doesn't make much of a > difference in practice. > > `You can't introduce another createXdgOutputManager() because it's not > overloaded`. He's adding an overload, I don't understand. > He's adding an overload, I don't understand. Bad wording, my bad. We are allowed to overload only methods that are already overloaded. From https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B > You cannot... > > For existing functions of any type: > > add an overload (BC, but not SC: makes ambiguous), adding overloads to > already overloaded functions is ok (any use of already needed a cast). REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
apol added inline comments. INLINE COMMENTS > davidedmundson wrote in display.h:296 > I can't make it explicit. It's not a constructor Passing an enum as const& is wrong although it doesn't make much of a difference in practice. `You can't introduce another createXdgOutputManager() because it's not overloaded`. He's adding an overload, I don't understand. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
zzag added inline comments. INLINE COMMENTS > display.h:297 > + */ > +XdgOutputManagerInterface *createXdgOutputManager(const > XdgOutputInterfaceVersion , QObject *parent = nullptr); > + You can't introduce another createXdgOutputManager() because it's not overloaded. You probably need to rename this method, e.g. createXdgOutputManager2. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: zzag, anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson added inline comments. INLINE COMMENTS > davidedmundson wrote in display.h:296 > I agree it's weird to take a const&, but it's what the others do I can't make it explicit. It's not a constructor REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson updated this revision to Diff 79127. davidedmundson added a comment. update REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28498?vs=79102=79127 BRANCH master REVISION DETAIL https://phabricator.kde.org/D28498 AFFECTED FILES autotests/client/test_xdg_output.cpp src/client/xdgoutput.h src/server/display.cpp src/server/display.h src/server/xdgoutput_interface.cpp src/server/xdgoutput_interface.h To: davidedmundson, #kwin Cc: anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson marked an inline comment as done. davidedmundson added inline comments. INLINE COMMENTS > apol wrote in display.h:287 > Should we deprecate the one without version? I'll add a doc. I don't want to waste time with the macro stuff given we know there's only one user. > anthonyfieroni wrote in display.h:296 > explicit, also take enum class by value. I agree it's weird to take a const&, but it's what the others do REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
anthonyfieroni added inline comments. INLINE COMMENTS > display.h:296 > + */ > +XdgOutputManagerInterface *createXdgOutputManager(const > XdgOutputInterfaceVersion , QObject *parent = nullptr); > + explicit, also take enum class by value. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: anthonyfieroni, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
apol added a comment. Some concerns at the documentation level. Looks good overall. INLINE COMMENTS > display.h:287 > */ > XdgOutputManagerInterface *createXdgOutputManager(QObject *parent = > nullptr); > Should we deprecate the one without version? > xdgoutput_interface.h:105 > * It should be set once before the first done call > - * @since 5.XDGOUTPUT > + * Version 2 only > + * @since 5.69 I'd write something like `Only effective since version UnstableV2`. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D28498 To: davidedmundson, #kwin Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28498: [xdgoutput] Explicitly set version of server interface
davidedmundson created this revision. davidedmundson added a reviewer: KWin. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. davidedmundson requested review of this revision. REVISION SUMMARY Implication being that when v2 is used name and description will be set appropriately. This brings us being spec-compliant if new wayland is used against old kwin. Resource check works by knowing that clients can't register a version newer than the client so setName will then no-op internally. TEST PLAN Test failed till I bumped it to V2. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D28498 AFFECTED FILES autotests/client/test_xdg_output.cpp src/client/xdgoutput.h src/server/display.cpp src/server/display.h src/server/xdgoutput_interface.cpp src/server/xdgoutput_interface.h To: davidedmundson, #kwin Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
