OpenBoard-org / OpenBoard

OpenBoard is a cross-platform interactive whiteboard application intended for use in a classroom setting.
https://openboard.ch/
GNU General Public License v3.0
2.34k stars 423 forks source link

fix: cast for UBApplication object during construction #1010

Closed letsfindaway closed 1 month ago

letsfindaway commented 3 months ago

This PR fixes some strange problems we recently had with OpenBoard using the wrong user data directory. Especially it helps (or even solves?) the problems described in

Vekhir commented 3 months ago

From a cursory grep on UBApplication::app() it seems like the result is most of the time assumed to be non-nullptr without checking. In the one case where it is checked, returning a nullptr would disable logging to a file, which doesn't look like it's the problem. If you've checked and it solves #1004, great. From a theoretic point of view (I don't have this issue), this doesn't seem related.

Of course, the issue remains that UBApplication::app() is uninitialised memory. Since everything depends on the app, perhaps just aborting the program might be the safest route. The logging could get a check like UBApplication::isInitialized (using dynamic_cast like here) to make it clear that it could be called during initialisation.

letsfindaway commented 3 months ago

From a cursory grep on UBApplication::app() it seems like the result is most of the time assumed to be non-nullptr without checking. In the one case where it is checked, returning a nullptr would disable logging to a file, which doesn't look like it's the problem. If you've checked and it solves #1004, great. From a theoretic point of view (I don't have this issue), this doesn't seem related.

It is related in the following way: grafik

Of course, the issue remains that UBApplication::app() is uninitialised memory. Since everything depends on the app, perhaps just aborting the program might be the safest route. The logging could get a check like UBApplication::isInitialized (using dynamic_cast like here) to make it clear that it could be called during initialisation.

I was thinking about better checks for nullptr but then did not do it for the following reasons:

So I don't think that any further check is necessary.

Vekhir commented 3 months ago

Thanks for detailing the causal relationship. I agree, this is the best course of action.

This shows how "set at first use" is flawed, but I can't think of anything better.