On 15.03.24 14:28, Volker Hilsheimer via Development wrote: >> On 15 Mar 2024, at 12:30, Marc Mutz via Development >> <development@qt-project.org> wrote: >> On 15.03.24 09:11, apoenitz wrote: >>> On Fri, Mar 15, 2024 at 07:16:59AM +0000, 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<QObject *>(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<bool(QObject*, QEVent*)> (except that std::function is horrible)? -- Marc Mutz <marc.m...@qt.io> 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
