[Development] Should QObject::event() be protected or public?

[Marc Mutz]

On 15.03.24 14:28, Volker Hilsheimer via Development wrote:
>> On 15 Mar 2024, at 12:30, Marc Mutz via Development <development at 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.mutz at 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