Re: [Development] Should QObject::event() be protected or public?
On Mon, Mar 18, 2024 at 02:00:06PM +0100, Giuseppe D'Angelo via Development wrote: > On 18/03/2024 13:34, André Somers wrote: > > While I know it's easy to work around, I sometimes find myself doing it > > anyway. To me, it signals what API is intended to be used in what way. > > That a class overrides `event` (or any other public virtual method) > > does not mean that that method is then intended to be called by a user > > of the class as the type you defined. That you overwrote it may just be > > an implementation detail. I think the methods you expose as "public" on > > an API are quite important*. They signal how the user is supposed to use > > an instance of your class. If you have methods in there that are just > > implementation details, then those don't fit. These methods are meant to > > be called by parts of the system that don't see your type as the actual > > type, but as something more basic: a QObject in this case. > > But I agree 100% here; this is typically realized in C++ by having the entry > point public and non-virtual, and have that dispatch to a protected virtual. The "Non-virtual interface" pattern. Been there, done that (in a different project), and sure, can be used and works. But I wouldn't call it "typical", as it imposes overhead for practically no gain even for new projects starting with that as the preferred approach, let alone to setups where this "needs" to be introduced later. > The whole problem we're discussing is that `event()` has been made public in > the base class and that means it's now public API of any QObject subclass, > whether they like it or not. :-( The subclasses can (and often do) make it protected. This is a pragmatic approach. It pretty much works and feels like NVI without having to write the redirection at the base level. Anyway, I would like to explicitly disgress here, as this is yet another instance of a pattern I see repeating again and again, and while I (grudgingly) accept that pragmatism is apparently not en vogue anymore, I still think there should be a limit to that: The actual problem here is /not/ whether or not QObject::event() should be public/overridden in derived classes, or not, whether NVI is good or not, but how much effort should be put into discussing and "solving" this kind of ivory-tower (non-)problems, and how well such a "solution" would scale to "all of Qt", instead of spending this energy and effort on fixing actual problems. Andre' PS: I had a quick look through the archive. There seems to be a pretty high correlatation between threads with more than ~20 responses and what I personally would call a "non-problem" to start with. Not 100%, but a good first approximation. PPS: If someone has doubts on the claim of things being non-problems I heartily invite them to come up with their own statistics on the actual effects of recent changes in the string zoo. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 18/03/2024 14:00, Giuseppe D'Angelo via Development wrote: On 18/03/2024 13:34, André Somers wrote: While I know it's easy to work around, I sometimes find myself doing it anyway. To me, it signals what API is intended to be used in what way. That a class overrides `event` (or any other public virtual method) does not mean that that method is then intended to be called by a user of the class as the type you defined. That you overwrote it may just be an implementation detail. I think the methods you expose as "public" on an API are quite important*. They signal how the user is supposed to use an instance of your class. If you have methods in there that are just implementation details, then those don't fit. These methods are meant to be called by parts of the system that don't see your type as the actual type, but as something more basic: a QObject in this case. But I agree 100% here; this is typically realized in C++ by having the entry point public and non-virtual, and have that dispatch to a protected virtual. Yes, such a design could have been used. But it was not, and it hasn't in many classes in Qt where overriding methods is part of the way to use it. I guess that could still be done, but not without a lot of pain. QAIM being a prime example. And I think there are also good reasons for that: it adds complication, another layer of indirection, another set of methods to document. To solve what problem exactly? The whole problem we're discussing is that `event()` has been made public in the base class and that means it's now public API of any QObject subclass, whether they like it or not. :-( You act as if I didn't understand that, while I started out with saying "While I know it's easy to work around [...]". I *know* we can still call virtual methods that were public in the base class but have been made protected or private in a subclass by just downcasting that subclass instance to the base class. But it stands out in the user-code as doing something that was, while possible, not intended by the class designer. Fine by me. But to me, that does not imply that I have to *advertise* that method on my public API in that subclass when it is re-implemented only for internal purposes, but you (and Marc) seem to be claiming that it should imply just that. To go back to the original question: should QObject::event() be protected or public? I think it's right that it's public. It is called from outside the class itself by parts of Qt that deal with event handling, so it belongs on the public API for QObject. Had it been protected, we would have needed to use friend, and IMO that's worse: you cannot control _which_ private methods or members are touched that way. So, in summary: I don't think we have a problem here. A public QObject::event advertises what it does, and it having been made protected in sub-classes is, IMO, not an actual problem. Cheers, André -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
To add a user perspective to the discussion: Please keep things simple and consistent. Consistency is great, especially in large code bases. Changing access modifiers for an existing interface in a derived class is confusing. Due to the way the documentation is structured, it’s also hard to see in the docs. Having to cast to the base class in order to access the interface is confusing. Do we bypass the implementation of the derived class? Of course not, but I wonder how many developers would stumble with that. I see two clean approaches: 1) Make event() public consistently 2) Create a new non-virtual public interface in Qt 6.x that can be used to call event(). Document that public access to event() will be removed in Qt7, and people using it should migrate to the new interface. Then, in Qt 7, make the virtual functions protected consistently. Option 2 would be the cleaner one, but some people may hate you for upsetting their codebase. Kind regards Robert This message and any attachments are solely for the use of the intended recipients. They may contain privileged and/or confidential information or other information protected from disclosure. If you are not an intended recipient, you are hereby notified that you received this email in error and that any review, dissemination, distribution or copying of this email and any attachment is strictly prohibited. If you have received this email in error, please contact the sender and delete the message and any attachment from your system. ANDRITZ HYDRO GmbH Rechtsform/ Legal form: Gesellschaft mit beschränkter Haftung / Corporation Firmensitz/ Registered seat: Wien Firmenbuchgericht/ Court of registry: Handelsgericht Wien Firmenbuchnummer/ Company registration: FN 61833 g DVR: 0605077 UID-Nr.: ATU14756806 Thank You -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 18 Mar 2024, at 14:00, Giuseppe D'Angelo via Development wrote: On 18/03/2024 13:34, André Somers wrote: While I know it's easy to work around, I sometimes find myself doing it anyway. To me, it signals what API is intended to be used in what way. That a class overrides `event` (or any other public virtual method) does not mean that that method is then intended to be called by a user of the class as the type you defined. That you overwrote it may just be an implementation detail. I think the methods you expose as "public" on an API are quite important*. They signal how the user is supposed to use an instance of your class. If you have methods in there that are just implementation details, then those don't fit. These methods are meant to be called by parts of the system that don't see your type as the actual type, but as something more basic: a QObject in this case. But I agree 100% here; this is typically realized in C++ by having the entry point public and non-virtual, and have that dispatch to a protected virtual. QObject::event() has been public since Qt 2.3 at least: https://doc.qt.io/archives/2.3/qobject.html And subclasses, by and large, always had those overrides as protected, e.g. https://doc.qt.io/archives/2.3/qwidget.html#6ff658 Was it because the old Trolls didn’t know better, or thought it would be a clever way to allow some specific use cases while preventing silly mistakes? The non-virtual interface pattern might have been around then already, and we do have a public entry point to QObject::event: QCoreApplication::send/postEvent, both of them linked to from the documentation. The function is also today still documented saying that it “receives events to an object"; the documentation doesn’t say “call this function if you want something to happen to the object”. Ironically, the QObject subclass in the snippet used also overrides event() as a public function (while the next snippet overrides eventFilter as a protected function). The whole problem we're discussing is that `event()` has been made public in the base class and that means it's now public API of any QObject subclass, whether they like it or not. :-( People can call QObject::event, as long as they cast to, or store their pointer as, a QObject, and as long as they know how to construct and correctly manage the life time of the respective QEvent subclass instance (unless they merely forward an existing one). I think that’s fine. Folks have evidently managed to do so for the last two decades, and the amount of foot-shooting seems to have been manageable. Perhaps it’s ok to expect that people that jump through those kinds of hoops read https://doc.qt.io/qt-6/eventsandfilters.html and know what they are doing. As I see it, nothing we can do to change this (make QObject::event protected, add a public NVI function) would improve Qt in a way that would justify the work. I’ll happily review a change to the documentation that fixes the snippet, and adds a sentence about using QCoreApplication::send- or postEvent instead, perhaps linking explicitly to https://doc.qt.io/qt-6/eventsandfilters.html. Maybe at some point the C++ committee will decide that compilers should warn if an override is not at least as strict as the virtual declaration. I’ll stand corrected then. Volker -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 18/03/2024 13:34, André Somers wrote: While I know it's easy to work around, I sometimes find myself doing it anyway. To me, it signals what API is intended to be used in what way. That a class overrides `event` (or any other public virtual method) does not mean that that method is then intended to be called by a user of the class as the type you defined. That you overwrote it may just be an implementation detail. I think the methods you expose as "public" on an API are quite important*. They signal how the user is supposed to use an instance of your class. If you have methods in there that are just implementation details, then those don't fit. These methods are meant to be called by parts of the system that don't see your type as the actual type, but as something more basic: a QObject in this case. But I agree 100% here; this is typically realized in C++ by having the entry point public and non-virtual, and have that dispatch to a protected virtual. The whole problem we're discussing is that `event()` has been made public in the base class and that means it's now public API of any QObject subclass, whether they like it or not. :-( -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence smime.p7s Description: S/MIME Cryptographic Signature -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Hey, On 18/03/2024 12:12, Giuseppe D'Angelo via Development wrote: Il 15/03/24 21:21, Jaroslaw Kobus via Development ha scritto: To the point: we are talking here about decreasing the visibility of a member function in a subclass. The virtuality of the method isn't relevant I guess, is it? It is relevant, since that's the whole problem: since event() is a virtual which is public in QObject, it's pointless to restrict it further in a subclass, because you can always place the call considering the object as-a QObject (=> upcast it) and bypass the restriction. Therefore, when one creates a QObject subclass with an event() override, then: * either they didn't know about the fact that it was public in QObject, and thought it was protected/private (because virtual functions should normally be protected), without think about the above C++ """feature""" at all, etc. (mistake in good faith); * or they were *deliberately* trying to change the access level, i.e. trying to *outsmart* C++, but that goes nowhere; making it more restrictive simply doesn't work (can be bypassed by upcasting, breaks generic code that codes against the interface, breaks subclasses). While I know it's easy to work around, I sometimes find myself doing it anyway. To me, it signals what API is intended to be used in what way. That a class overrides `event` (or any other public virtual method) does not mean that that method is then intended to be called by a user of the class as the type you defined. That you overwrote it may just be an implementation detail. I think the methods you expose as "public" on an API are quite important*. They signal how the user is supposed to use an instance of your class. If you have methods in there that are just implementation details, then those don't fit. These methods are meant to be called by parts of the system that don't see your type as the actual type, but as something more basic: a QObject in this case. It's not a matter of "outsmarting" the compiler, or something like that. It's telling the user of the class which method he is supposed to focus on. I find myself doing these kinds of things for instance with QAIM, which as quite a big API to re-implement in some cases. Those are all public in the base class, but that does not mean that the person using my customized type should be fiddling with those methods: they are for consumption by views and proxy models and other bits that operate on QAIM instances. So, I tend to make these overridden methods protected instead, and only keep the higher-level methods that make sense in terms of the application API public. You also seem to be overlooking that you can change the access level of all methods of the base class by not using public inheritance, but using protected or private. Those don't get much use, but I would say that it's an intentional feature. Cheers, André *) And in the same way, that the "protected" section is the API available for someone subclassing your class. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Giuseppe D'Angelo (18 March 2024 12:12) > Therefore, when one creates a QObject subclass with an event() > override, then: > > * either they didn't know about the fact that it was public in > QObject, and thought it was protected/private (because virtual > functions should normally be protected), without think about the > above C++ """feature""" at all, etc. (mistake in good faith); > > * or they were *deliberately* trying to change the access level, i.e. > trying to *outsmart* C++, but that goes nowhere; making it more > restrictive simply doesn't work (can be bypassed by upcasting, > breaks generic code that codes against the interface, breaks > subclasses). Or they could be thinking: "surely that *should* be protected in QObject, so I'll make it protected in my derived class so that *when* someone gets round to (at a major version change) making QObject::event() protected, this overload shall be ready-protected in support of that." Whether that's a good plan or not is another story, but it's another possible motive. Eddy. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Il 15/03/24 21:21, Jaroslaw Kobus via Development ha scritto: To the point: we are talking here about decreasing the visibility of a member function in a subclass. The virtuality of the method isn't relevant I guess, is it? It is relevant, since that's the whole problem: since event() is a virtual which is public in QObject, it's pointless to restrict it further in a subclass, because you can always place the call considering the object as-a QObject (=> upcast it) and bypass the restriction. Therefore, when one creates a QObject subclass with an event() override, then: * either they didn't know about the fact that it was public in QObject, and thought it was protected/private (because virtual functions should normally be protected), without think about the above C++ """feature""" at all, etc. (mistake in good faith); * or they were *deliberately* trying to change the access level, i.e. trying to *outsmart* C++, but that goes nowhere; making it more restrictive simply doesn't work (can be bypassed by upcasting, breaks generic code that codes against the interface, breaks subclasses). I've never used it personally, but the fact that C++ language designers let it compile, even without a warning, makes me wonder that there were some reasons for it. C++ has also 30? 40? years of crust. Why does C++ even allow different accessibility levels for different overrides to begin with? My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence smime.p7s Description: Firma crittografica S/MIME -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Sat, Mar 16, 2024 at 4:01 PM Marc Mutz via Development < development@qt-project.org> wrote: > Function != variable ;-) > Granted, they're different and using declarations are intended for and work for _both_ functions and variables. I was simply providing some context where this syntax is actually quite useful, hence the usage of "You can ALSO use this for [...]". That aside, is the principal discussion theoretical as it seem to have strayed quite a lot? I get your argument(s) and I mostly agree, in principle that is. Yet, to give you a counter example: if you `protected` inherited some class you might want to hide some of its (virtual) methods for derived classes and make them private. This a valid use case I believe, and is probably why the language allows you to do so, rightfully - I imagine some (edge) cases where you may want to do this sort of thing. So to summarize my opinion: Should `QObject::event` be made `protected` - likely, yes - but you would need to collect the use cases for the public method and provide a migration path, to play nice with the users. Should this be discouraged if possible - likely, yes. Should this guideline be made a rule, a.k.a. chisel it in stone - hell no. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 16.03.24 01:47, Konstantin Shegunov wrote:
> On Fri, Mar 15, 2024 at 10:48 PM Marc Mutz via Development
> mailto:development@qt-project.org>> wrote:
>
> The member variable thing sounds very wrong. I'd be surprised if it
> compiled.
[...]
> struct A
> {
> int foo(int);
> };
> struct B
> {
> double foo(double);
> };
> struct C : A, B
> {
> using A::foo;
> using B::foo;
> };
>
> Which allows C to have `foo()` properly overloaded.
Function != variable ;-)
--
Marc Mutz
Principal Software Engineer
The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io
Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B
--
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Fri, Mar 15, 2024 at 10:48 PM Marc Mutz via Development <
development@qt-project.org> wrote:
> The member variable thing sounds very wrong. I'd be surprised if it
> compiled.
Then you'd be surprised.
> If it does, it's probably something new in C++23 or 26 whereby
> you can use `using` for more general renaming [...]
If I'm not mistaken it's a C++17 feature. You can also use this to 'pull'
specific methods out of specific bases (e.g. if there's a name
clash/ambiguity), not only for changing access scope.
E.g.
struct A
{
int foo(int);
};
struct B
{
double foo(double);
};
struct C : A, B
{
using A::foo;
using B::foo;
};
Which allows C to have `foo()` properly overloaded.
--
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 15.03.24 21:21, Jaroslaw Kobus via Development wrote:
[...]
> Just found some reasoning, though not sure how much valid it is:
> https://www.learncpp.com/cpp-tutorial/hiding-inherited-functionality/
>
> Probably there are more resources about this topic in the internet.
That page doesn't look very trustworthy.
The first example is valid. One can make something public that was
protected in a base class, because it doesn't violate Liskov's
Substitution Principle: One can still do everything one can do with the
base class with the derived class, too. One can just do more with the
derived class, which is pretty common (one can get text() from a
QLineEdit, but not from a QWidget (well, except via its Q_PROPERTY)).
The member variable thing sounds very wrong. I'd be surprised if it
compiled. If it does, it's probably something new in C++23 or 26 whereby
you can use `using` for more general renaming, like
struct insert_result {
iterator it;
bool inserted;
using first = it; // was std::pair before, keep .first working
using second = inserted;
};
I saw a proposal for that floating around some time ago, but don't know
the status of it.
In general, just because C++ lets one do something doesn't mean it's a
good idea. Apart from the C ballast, C++ itself is a language with a
huge design-space, and four decades of cruft. Only very few planes in
that design space make sense in general, they change with each C++
version, and even fewer of them make sense in any given project context.
There's a reason why CppCoreGuidelines are so huge and why TTLOFIR is
growing so rapidly: C++ projects need hundreds or thousands of rules to
tailor C++'s vast design space to something that's acceptable for the
project. Do the work, or die from technical debt. The core of Qt
(qt.git) is pretty consistent, still, but we have dozens of modules, and
hundreds of contributors now of extremely varying experience, and C++
has evolved a lot since C++98, so I think we need to codify a lot more
stuff than we have done in the past (and, where possible, have tools
check it). Look at all the google-specific checks in clang-tidy for example.
Thanks,
Marc
--
Marc Mutz
Principal Software Engineer
The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io
Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B
--
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
> From: Development on behalf of Giuseppe > D'Angelo via Development > Sent: Friday, March 15, 2024 8:58 PM > To: development@qt-project.org > Subject: Re: [Development] Should QObject::event() be protected or public? > > Il 15/03/24 19:22, Giuseppe D'Angelo via Development ha scritto: >> Il 15/03/24 19:17, Jaroslaw Kobus via Development ha scritto: >>> +1. Typically, the designer of a subclass knows what he is doing. But it >>>also happens that users of this class know better how to use it :) >> I'm not sure what this means. >> >> If you override `event()` (public in QObject) as e.g. protected, it's >> either a mistake in good faith (you thought it was protected all along), >> but it's never a "I know better". You clearly*don't* know better, as >> you don't know C++. > > I'm really sorry, I just realized that this can be read in a very > offensive way: the "you" above was in *no* way directed to Jaroslaw > Kobus. It was a generic "you" -- as opposed as "I know better", for a > generic subject "I", not necessarily the writer. I apologize to > Jaroslaw if this caused a misunderstanding. Don't worry, Peppe, I didn't take it personally, all is fine :) To the point: we are talking here about decreasing the visibility of a member function in a subclass. The virtuality of the method isn't relevant I guess, is it? I've never used it personally, but the fact that C++ language designers let it compile, even without a warning, makes me wonder that there were some reasons for it. Just found some reasoning, though not sure how much valid it is: https://www.learncpp.com/cpp-tutorial/hiding-inherited-functionality/ Probably there are more resources about this topic in the internet. Regards Jarek -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Il 15/03/24 19:22, Giuseppe D'Angelo via Development ha scritto: Il 15/03/24 19:17, Jaroslaw Kobus via Development ha scritto: +1. Typically, the designer of a subclass knows what he is doing. But it also happens that users of this class know better how to use it :) I'm not sure what this means. If you override `event()` (public in QObject) as e.g. protected, it's either a mistake in good faith (you thought it was protected all along), but it's never a "I know better". You clearly*don't* know better, as you don't know C++. I'm really sorry, I just realized that this can be read in a very offensive way: the "you" above was in *no* way directed to Jaroslaw Kobus. It was a generic "you" -- as opposed as "I know better", for a generic subject "I", not necessarily the writer. I apologize to Jaroslaw if this caused a misunderstanding. -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence smime.p7s Description: Firma crittografica S/MIME -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 15/3/24 19:57, Jean-Michaël Celerier wrote: There's nothing more frustrating that instantiating a C++ type on the stack like MyType foo; and having to call a function with: static_cast(foo).someVirtualMethod(); Or: foo.Base::someVirtualMethod(); [..] -- Ahmad Samir OpenPGP_signature Description: OpenPGP digital signature -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Il 15/03/24 19:17, Jaroslaw Kobus via Development ha scritto: +1. Typically, the designer of a subclass knows what he is doing. But it also happens that users of this class know better how to use it :) I'm not sure what this means. If you override `event()` (public in QObject) as e.g. protected, it's either a mistake in good faith (you thought it was protected all along), but it's never a "I know better". You clearly *don't* know better, as you don't know C++. My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence smime.p7s Description: Firma crittografica S/MIME -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
> From: Development on behalf of Thiago > Macieira > Sent: Friday, March 15, 2024 7:03 PM > To: development@qt-project.org > Subject: Re: [Development] Should QObject::event() be protected or public? > > On Friday, 15 March 2024 10:09:31 PDT Marc Mutz via Development wrote: >> I like simple rules. "Overrides should have the same access level as the >> initial virtual function." is a simple rule. > > Amend that to "... unless there's an explicit reason not to". With that, I'm > ok. As you can still access by casting to a base, that had better be a good > reason. +1. Typically, the designer of a subclass knows what he is doing. But it also happens that users of this class know better how to use it :) Jarek -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Friday, 15 March 2024 11:03:19 PDT Thiago Macieira wrote: > On Friday, 15 March 2024 10:09:31 PDT Marc Mutz via Development wrote: > > I like simple rules. "Overrides should have the same access level as the > > initial virtual function." is a simple rule. > > Amend that to "... unless there's an explicit reason not to". With that, I'm > ok. As you can still access by casting to a base, that had better be a good > reason. Which actually points to a reason why you'd do it: to make it *more* accessible (make public, or make a private become protected). -- Thiago Macieira - thiago.macieira (AT) intel.com Principal Engineer - Intel DCAI Cloud Engineering smime.p7s Description: S/MIME cryptographic signature -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Friday, 15 March 2024 10:09:31 PDT Marc Mutz via Development wrote: > I like simple rules. "Overrides should have the same access level as the > initial virtual function." is a simple rule. Amend that to "... unless there's an explicit reason not to". With that, I'm ok. As you can still access by casting to a base, that had better be a good reason. -- Thiago Macieira - thiago.macieira (AT) intel.com Principal Engineer - Intel DCAI Cloud Engineering smime.p7s Description: S/MIME cryptographic signature -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
There's nothing more frustrating that instantiating a C++ type on the stack like MyType foo; and having to call a function with: static_cast(foo).someVirtualMethod(); because "The base class is the interface" and all the overriding methods are marked private. This is just adding bureaucracy for absolutely no good reason and doesn't help *anyone*. On Fri, Mar 15, 2024 at 1:32 PM Christian Kandeler via Development < development@qt-project.org> wrote: > On 3/15/24 18:09, Marc Mutz via Development wrote: > > I like simple rules. "Overrides should have the same access level as the > > initial virtual function." is a simple rule. > > But it makes no sense in general. The base class is the interface, and > overrides should have the least possible visibility for their purpose. > > > Christian > > -- > Development mailing list > Development@qt-project.org > https://lists.qt-project.org/listinfo/development > -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 15.03.24 18:31, Christian Kandeler via Development wrote: > On 3/15/24 18:09, Marc Mutz via Development wrote: >> I like simple rules. "Overrides should have the same access level as the >> initial virtual function." is a simple rule. > > But it makes no sense in general. The base class is the interface, and > overrides should have the least possible visibility for their purpose. Let me turn that around and ask you to explain why the specific static type should matter when calling a virtual function: MyWidget *myw = new MyWidget; // ¹ QWidget *id = myw; QObject *obj = wid; obj->event(e); // OK wid->event(e); // ERROR: protected myw-.event(e); // OK, public again "Normal" functions never behave that way. Why should virtual functions behave so erratically? Users look at the QObject docs, see that there's a public event() function and for some reason decide that's the function to call. QWidget is-a QObject, so by the Liskov Substitution Principle, you should be able to do anything to a QWidget than you can do to a QObject. So why can't I call event() on it? ¹ from src/corelib/doc/snippets/events/events.cpp (iow: our docs) Thanks, Marc -- Marc Mutz Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 3/15/24 18:09, Marc Mutz via Development wrote: I like simple rules. "Overrides should have the same access level as the initial virtual function." is a simple rule. But it makes no sense in general. The base class is the interface, and overrides should have the least possible visibility for their purpose. Christian -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 15.03.24 14:28, Volker Hilsheimer via Development wrote:
>> On 15 Mar 2024, at 12:30, Marc Mutz via Development
>> wrote:
>> On 15.03.24 09:11, apoenitz wrote:
>>> On Fri, Mar 15, 2024 at 07:16:59AM +, Marc Mutz via Development wrote:
>> [...]
Please note that this means that any override (and all new QObject
classes should contain one, since, in case we ever need it, it might not
be possible to add it after the fact) should also be public.
>>>
>>> Neither is a necessary consequence, and I don't think this new rule
>>> would be helpful.
>>
>> I have explained this here and over in TTLOFIR. If people choose to not
>> read, or, if read, choose not to understand, or, if understood, choose
>> to not accept, there's nothing more that I can do.
>
> I have read, but not understood, what or where “TTLOFIR” is.
Don't worry, I'll repeat TTLOFIR until the last Qt developer knows the
acronym: https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews :)
>>> What kind of setups do you expect where an event() override in the
>>> middle of the inheritance chain needs to be called from outside to
>>> warrant this rule requiring class-implementor to regularly write code
>>> that's unlikely (and I claim: Never) needed?
>>
>> It's not my job to argue why we should keep breaking a very general C++
>> rule, one most C++ developers probably don't know _can_ be broken, but
>> I'll throw you a bone: qcompleter.cpp
>
> That there exists code that calls QObject::event directly, whether because it
> can (and the author didn’t know about QCoreApplication::sendEvent), or
> because it has to (to avoid recursion when using sendEvent), doesn’t make it
> any less of a smell.
>
> And we evidently need to have a public QObject::event so that code that must
> call it (for whatever reason) has a way to do so. Some smells have valid
> reasons to exist, and making them visible in code by requiring
>
> (static_cast(d->widget))->event(ke);
>
> (unnecessary parenthesis presumably not required for it to raise eyebrows in
> code review) is a good thing.
>
> But I still don’t see a reason why we now need to start arguing about a
> pattern that has been used in Qt quite consistently for 30 years, i.e. making
> overrides of event handlers, including QObject::event and
> QObject::eventFilter, protected.
>
> Volker
>
> PS: and indeed, just that something has been done for 30 years is not an
> argument in itself; that it’s a pattern that has worked, has been accepted,
> and has prevented mistakes however is.
I like simple rules. "Overrides should have the same access level as the
initial virtual function." is a simple rule. Clang-tidy probably has a
checker for that already, but if not, it's trivially written. Qt is very
large, if there's tooling that we can use to encode rules, then that's
what we should be doing, IMHO. Esp. now that TQtC owns Axivion.
Speaking from API-review experience here.
But, yes, by majority vote, in Qt we currently have "Overrides are
generally the same access level as the initial virtual function, unless
the developer didn't pay attention, didn't care (we have private API
where everything is public e.g.) or unless the function is
QObject::event() or QObject::eventFilter(), in which case
reimplementations should be protected to make code calling the function
on derived classes uglier. Check back often, in case we find more
exceptions."
Clang-tidy/Axivion for sure don't have a check for that, and it's not
quite so trivial to implement.
So, back to QObject::event() as protected function. Should we then add
non-virtual QObject::process/filterEvent(QEvent*) (to be bikeshed) that
call protected event()/eventFilter()?
Thanks,
Marc
PS: Should we finally lift the restriction that event filters must be
QObject's? Could have been
class QEventFilter {
virtual ~QEventFilter();
virtual bool filter(QObject *o, QVent *e) const = 0;
};
Or, these days, std::function (except that
std::function is horrible)?
--
Marc Mutz
Principal Software Engineer
The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io
Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B
--
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
> On 15 Mar 2024, at 12:30, Marc Mutz via Development > wrote: > On 15.03.24 09:11, apoenitz wrote: >> On Fri, Mar 15, 2024 at 07:16:59AM +, Marc Mutz via Development wrote: > [...] >>> Please note that this means that any override (and all new QObject >>> classes should contain one, since, in case we ever need it, it might not >>> be possible to add it after the fact) should also be public. >> >> Neither is a necessary consequence, and I don't think this new rule >> would be helpful. > > I have explained this here and over in TTLOFIR. If people choose to not > read, or, if read, choose not to understand, or, if understood, choose > to not accept, there's nothing more that I can do. I have read, but not understood, what or where “TTLOFIR” is. >> What kind of setups do you expect where an event() override in the >> middle of the inheritance chain needs to be called from outside to >> warrant this rule requiring class-implementor to regularly write code >> that's unlikely (and I claim: Never) needed? > > It's not my job to argue why we should keep breaking a very general C++ > rule, one most C++ developers probably don't know _can_ be broken, but > I'll throw you a bone: qcompleter.cpp That there exists code that calls QObject::event directly, whether because it can (and the author didn’t know about QCoreApplication::sendEvent), or because it has to (to avoid recursion when using sendEvent), doesn’t make it any less of a smell. And we evidently need to have a public QObject::event so that code that must call it (for whatever reason) has a way to do so. Some smells have valid reasons to exist, and making them visible in code by requiring (static_cast(d->widget))->event(ke); (unnecessary parenthesis presumably not required for it to raise eyebrows in code review) is a good thing. But I still don’t see a reason why we now need to start arguing about a pattern that has been used in Qt quite consistently for 30 years, i.e. making overrides of event handlers, including QObject::event and QObject::eventFilter, protected. Volker PS: and indeed, just that something has been done for 30 years is not an argument in itself; that it’s a pattern that has worked, has been accepted, and has prevented mistakes however is. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On 15.03.24 09:11, apoenitz wrote: > On Fri, Mar 15, 2024 at 07:16:59AM +, Marc Mutz via Development wrote: [...] >> Please note that this means that any override (and all new QObject >> classes should contain one, since, in case we ever need it, it might not >> be possible to add it after the fact) should also be public. > > Neither is a necessary consequence, and I don't think this new rule > would be helpful. I have explained this here and over in TTLOFIR. If people choose to not read, or, if read, choose not to understand, or, if understood, choose to not accept, there's nothing more that I can do. > What kind of setups do you expect where an event() override in the > middle of the inheritance chain needs to be called from outside to > warrant this rule requiring class-implementor to regularly write code > that's unlikely (and I claim: Never) needed? It's not my job to argue why we should keep breaking a very general C++ rule, one most C++ developers probably don't know _can_ be broken, but I'll throw you a bone: qcompleter.cpp Thanks, Marc -- Marc Mutz Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Fri, Mar 15, 2024 at 07:16:59AM +, Marc Mutz via Development wrote: > Summary as of 2024-3-14 EOD: QObject::event() has to stay public. Ok. > Please note that this means that any override (and all new QObject > classes should contain one, since, in case we ever need it, it might not > be possible to add it after the fact) should also be public. Neither is a necessary consequence, and I don't think this new rule would be helpful. What kind of setups do you expect where an event() override in the middle of the inheritance chain needs to be called from outside to warrant this rule requiring class-implementor to regularly write code that's unlikely (and I claim: Never) needed? Please note that this thread was started with the assumption that not even the base QObject::event() needs to be callable from the outside, [and also that, in principle, for truly hard cases, one can legally sneak out of the 'protected' jail via litb's trick using ISO/EIC 14882:2011 14.7.2(12)] Andre' > Thanks, > Marc > > On 13.03.24 08:58, Marc Mutz via Development wrote: > > Hi, > > > > In API review, we detected some overrides that changed the access > > specifier vis-a-vis the original virtual function > > (https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes > > Item 5.3). > > > > One of them was a protected reimplementation of QObject::event() (which > > itself is public). > > > > The reason why QObject::event() is public seems lost to history, but the > > feeling in the review comments¹ was that it should have been protected > > from the get-go (and QObject befriended by whoever delivers events). > > > > If you see any reason for QObject::event() to stay public in Qt 7 and > > not become protected, please speak up before we fork Qt 7.0 :) > > > > Thanks, > > Marc > > > > ¹ > > https://codereview.qt-project.org/c/qt/qtdeclarative/+/528290/comment/f51938ca_fd065a18/ > > > -- > Marc Mutz > Principal Software Engineer > > The Qt Company > Erich-Thilo-Str. 10 12489 > Berlin, Germany > www.qt.io > > Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen > Sitz der Gesellschaft: Berlin, > Registergericht: Amtsgericht Charlottenburg, > HRB 144331 B > > -- > Development mailing list > Development@qt-project.org > https://lists.qt-project.org/listinfo/development -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Summary as of 2024-3-14 EOD: QObject::event() has to stay public. Please note that this means that any override (and all new QObject classes should contain one, since, in case we ever need it, it might not be possible to add it after the fact) should also be public. Thanks, Marc On 13.03.24 08:58, Marc Mutz via Development wrote: > Hi, > > In API review, we detected some overrides that changed the access > specifier vis-a-vis the original virtual function > (https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes > Item 5.3). > > One of them was a protected reimplementation of QObject::event() (which > itself is public). > > The reason why QObject::event() is public seems lost to history, but the > feeling in the review comments¹ was that it should have been protected > from the get-go (and QObject befriended by whoever delivers events). > > If you see any reason for QObject::event() to stay public in Qt 7 and > not become protected, please speak up before we fork Qt 7.0 :) > > Thanks, > Marc > > ¹ > https://codereview.qt-project.org/c/qt/qtdeclarative/+/528290/comment/f51938ca_fd065a18/ > -- Marc Mutz Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
On Wed, 13 Mar 2024 07:58:20 + Marc Mutz via Development wrote: > Hi, > > In API review, we detected some overrides that changed the access > specifier vis-a-vis the original virtual function > (https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes > Item 5.3). > > One of them was a protected reimplementation of QObject::event() > (which itself is public). > > The reason why QObject::event() is public seems lost to history, but > the feeling in the review comments¹ was that it should have been > protected from the get-go (and QObject befriended by whoever delivers > events). > > If you see any reason for QObject::event() to stay public in Qt 7 and > not become protected, please speak up before we fork Qt 7.0 :) > > Thanks, > Marc > > ¹ > https://codereview.qt-project.org/c/qt/qtdeclarative/+/528290/comment/f51938ca_fd065a18/ > Telegram Desktop has a use-case that it has a menu bar with actions having shortcuts like QKeySequence::Copy which are implemented as sending a QKeyEvent press/release sequence with the appropriate (same) shortcut to the currently focused widget. This is done using QObject::event directly to avoid a recursion as the menu bar hijacks the QKeyEvents if QCoreApplication::sendEvent is used. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Most probably making it protected (temporarily) and trying to build everything else (including QtCreator) may reveal the original reason for being public. Jarek From: Development on behalf of Marc Mutz via Development Sent: Wednesday, March 13, 2024 8:58 AM To: development@qt-project.org Subject: [Development] Should QObject::event() be protected or public? Hi, In API review, we detected some overrides that changed the access specifier vis-a-vis the original virtual function (https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes Item 5.3). One of them was a protected reimplementation of QObject::event() (which itself is public). The reason why QObject::event() is public seems lost to history, but the feeling in the review comments¹ was that it should have been protected from the get-go (and QObject befriended by whoever delivers events). If you see any reason for QObject::event() to stay public in Qt 7 and not become protected, please speak up before we fork Qt 7.0 :) Thanks, Marc ¹ https://codereview.qt-project.org/c/qt/qtdeclarative/+/528290/comment/f51938ca_fd065a18/ -- Marc Mutz Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Should QObject::event() be protected or public?
Il 13/03/24 08:58, Marc Mutz via Development ha scritto: If you see any reason for QObject::event() to stay public in Qt 7 and not become protected, please speak up before we fork Qt 7.0 :) I am *positive* that there's code out there that just calls event() to deliver an event. (Maybe there's even a use case -- attempt a deliver from an event filter.) So, basically, we dag our own grave there, this has to stay public for the future ... My 2 c, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence smime.p7s Description: Firma crittografica S/MIME -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
[Development] Should QObject::event() be protected or public?
Hi, In API review, we detected some overrides that changed the access specifier vis-a-vis the original virtual function (https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes Item 5.3). One of them was a protected reimplementation of QObject::event() (which itself is public). The reason why QObject::event() is public seems lost to history, but the feeling in the review comments¹ was that it should have been protected from the get-go (and QObject befriended by whoever delivers events). If you see any reason for QObject::event() to stay public in Qt 7 and not become protected, please speak up before we fork Qt 7.0 :) Thanks, Marc ¹ https://codereview.qt-project.org/c/qt/qtdeclarative/+/528290/comment/f51938ca_fd065a18/ -- Marc Mutz Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
