[KDecoration] Restore application menu button
ClosedPublic

Authored by broulik on Oct 17 2016, 3:53 PM.

Diff Detail

Repository
R129 Window Decoration Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik updated this revision to Diff 7466.Oct 17 2016, 3:53 PM
broulik retitled this revision from to WIP: Restore application menu button.
broulik updated this object.
broulik edited the test plan for this revision. (Show Details)
broulik added a reviewer: Plasma.
broulik set the repository for this revision to R129 Window Decoration Library.
Restricted Application added a project: Plasma. · View Herald TranscriptOct 17 2016, 3:53 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik updated this revision to Diff 7574.Oct 20 2016, 1:30 PM
broulik retitled this revision from WIP: Restore application menu button to [KDecoration] Restore application menu button.

Clean up a bit and better wording

graesslin added inline comments.
src/private/decoratedclientprivate.h
79

As that's adding new virtuals we either need to move that into a subclass or increase the so version of the private lib (or just ignore, but then distros complain).

graesslin added inline comments.
src/private/decoratedclientprivate.h
79

@sitter with your distro hat on: what do you think works best? We have here a public api with a "private" library which is used by KWin.

sitter added inline comments.Oct 20 2016, 2:19 PM
src/private/decoratedclientprivate.h
79

Even if the API is public... plasma libs have no ABI stability promise so I think it's easiest to simply bump the so version. That way distributions can't accidently build kwin against the wrong version of libkdecoration and then have odd vtable crashes. And since this is only BIC but not SIC it's only a minor rebuild inconvenience to anyone who's using libkdecoration's public API.

That said, a fully backwards compatible addition using a subclass is of course always nicer :)

What shall we do with this (the new virtual) now?

broulik updated this revision to Diff 9772.Jan 5 2017, 4:44 PM
  • Pass through actionId argument

I'm for dedicated sub-class. Reason is that this can result in an annoying transition with e.g. breeze compiled against the old and kwin against the new. Let's subclass and cast correctly

broulik updated this revision to Diff 9782.Jan 5 2017, 11:06 PM
  • Introduce subclass (admittedly with a quite a long name) to avoid adding new virtuals in exported class
broulik updated this revision to Diff 9783.Jan 5 2017, 11:12 PM
  • Add basic autotest that verifies that clicking the app menu button results in an application menu request
davidedmundson added inline comments.
src/decoration.cpp
201

I don't get this? We've replaced one ABI crash with throwing a runtime exception on calling a pure virtual, which isn't going to be much better.

We either want:

if d->metaObject() == ApplicationMenuEnabledDecoratedClientPrivate::staticMetaObject()

Or to make the methods in the other class non pure virtuals.

Martin, see David's comment - what should I do? :)

graesslin added inline comments.Jan 10 2017, 4:10 PM
src/decoration.cpp
201

I agree, static_cast doesn't make sense. I would do a dynamic_cast/qobject_cast and check for null.

broulik added inline comments.Jan 10 2017, 5:25 PM
src/decoration.cpp
201

DecoratedClientPrivate is not a QObject, so only option is dynamic_cast

broulik updated this revision to Diff 9996.Jan 10 2017, 5:32 PM

Use dynamic_cast

maybe I miss something, but that's still static_cast

broulik updated this revision to Diff 10003.Jan 10 2017, 7:16 PM
  • Fix one missing static_cast → dynamic_cast
graesslin accepted this revision.Jan 10 2017, 7:41 PM
graesslin added a reviewer: graesslin.
This revision is now accepted and ready to land.Jan 10 2017, 7:41 PM
This revision was automatically updated to reflect the committed changes.