Orientation discussion

irc.txt

13:31:22 torarne: ablasche: eskil: QScreen::orientation is documented to never return Qt::PrimaryOrientation. I think we should change that. Qt::PrimaryOrientation is _exactly_ what it should return if you don't have a mask set
13:32:11 eskil: torarne: I guess the return value when there's no mask is currently undefined, so I don't see any problem changing that
13:32:51 torarne: 5.4 documented behaviour change? 
13:33:00 ablasche: torarne: agree. This reminds me that my QuickScreen patch shouyld probably filter primaryOrientation out too
13:33:07 ablasche: torarne: yes
13:37:38 ablasche: or not since it is 0
13:42:20 VioletGiraffe: Greetings. Is this a right place to ask about bugs in Qt?
13:42:28 torarne: ablasche: filter how?
13:43:55 ablasche: torarne: never mind. nothing to filter since primary is 0 anyway
13:44:33 andre_: VioletGiraffe: you can try
13:47:29 torarne: ablasche: eskil: the one use-case that the change will break is if you enable the mask, let the device rotate, then disable it again, and expect to read out the "old" orientation from that point on
13:47:57 eskil: torarne: Is that a legitimate use case or just something someone might do?
13:48:13 torarne: i don't think so, it should be cached by the app if that's the sort of on/off behavior you want
13:49:09 eskil: torarne: What do we gain by changing it?
13:49:51 torarne: it makes the API clearer, without adding a runtime-warning that "hey, you are using the api wrong"
13:49:57 VioletGiraffe: andre_: I wonder if there's any way to attract attention to my bug report that has been opened for a while but hasn't seen action: https://bugreports.qt-project.org/browse/QTBUG-39313
13:49:58 qt_gerrit: Menu bar doesn't handle mouse click properly when there is a QGLWidget/native widget - https://bugreports.qt-project.org/browse/QTBUG-39313
13:50:18 eskil: torarne: Couldn't the default be PrimaryOrientation, but once it has been set once, it stays the same?
13:50:31 eskil: torarne: Seems like we shouldn't change the behaviour unless we have a reason
13:51:00 eskil: torarne: People who are setting and clearing the mask probably understand the property anyway
13:51:12 torarne: i think undefined -> defined in one way is better than defined in many ways
13:51:41 torarne: eskil: QScreen:orienation is _what is the orienation now_, not what was it last time
13:51:51 torarne: or, it could be, but that makes the api even more fuzzy
13:51:55 eskil: torarne: Not if the mask is set to never update it
13:52:21 eskil: torarne: You are adding the need for a cache in applications that want the old behavior for no other reason than that you want to make the definition simpler
marcel is now known as Guest1890 (13:52:43)
13:52:48 eskil: torarne: I would say the need for a simpler definition is not a good enough reason to break behaviour
13:53:56 torarne: eskil: i think the number of clients that explicitly rely on this behavior is <= 0, and the number of clients that accidentally get a stale/cached orientation is >= 0
13:53:57 eskil: torarne: I also think that always returning PrimaryOrientation to the people who think the orientation is updated is not helping them very much and equal to defaulting to PrimaryOrientation. Everyone will still have to read the docs to understand how this works
13:54:18 torarne: https://codereview.qt-project.org/#/c/95661/
13:54:27 eskil: torarne: The number of clients is potentially < 0?
13:54:33 torarne: yepp 
13:54:38 torarne: <= 1 
13:55:05 torarne: you got my point 
13:55:26 eskil: torarne: I think there's always a chance of someone depending on behaviour we have in Qt
13:55:55 eskil: torarne: And I don't think we gain anything by changing it
13:55:56 torarne: eskil: true, which is why we have change-log-announcements when we do change behavior
13:56:28 torarne: eskil: as in you don't think it makes sense at all (in a qt6 world), or as a behavior change?
13:56:37 torarne: eskil: https://codereview.qt-project.org/#/c/95661/
13:57:32 eskil: torarne: No, ok, it makes sense, but in practice I don't think it's going to help anyone and it has the potential of breaking behaviour for someone
13:57:45 eskil: torarne: I think the people who are confused by this will still have to read the docs which will instruct them to use the mask
13:58:06 eskil: torarne: But for Qt 6, yes, it might be good to have clearer definition and maybe get rid of the property entirely
14:00:00 torarne: I still think that undefined behavior is significantly worse than anything else, but okey
14:00:23 eskil: torarne: Lets define the behaviour then 
14:00:45 eskil: torarne: If we go with your patch you have to connect orientationUpdateMaskChanged to orientationChanged
14:01:02 eskil: torarne: Otherwise it will go out of sync
mck182|lunch is now known as mck182 (14:01:09)
14:01:09 torarne: eskil: updateFilteredScreenOrientation -> reportScreenOrientationChange
14:01:17 torarne: -> emit orientationChanged
14:01:23 torarne: QGuiApplicationPrivate
14:01:28 eskil: torarne: Ok
14:02:14 eskil: torarne: I think the property actually becomes simpler if we say: The default is PrimaryOrientation. The property is only updated when the update mask is set.
14:02:55 eskil: torarne: That's also a pretty valid definition of the behaviour
14:03:29 zecke: Any QtSql/Postgres expert around? How/When to detect that the DB connection is gone, re-connect and re-create the prepared statements?
14:04:34 torarne: eskil: valid yes, simpler no  we move from never returning Qt::PrimaryOrienation to returning it (when the mask is not set &&& the orientartion has not been set yet by the platform while the mask was on)
14:04:57 torarne: eskil: vs never returning Qt::PrimaryOrienation to returning it (when the mask is not set)
14:05:17 maelcum: hi. is it correct that font shaping *always* goes through harfbuzz, with input from platform-specfic apis. it looks so to me, but maybe i've missed some code path.
14:05:32 maelcum: (insert question mark after first sentence)
14:05:33 torarne: eskil: but i agree it is slighty more safe for whoever might rely on the caching. 
14:05:47 eskil: torarne: I don't think one is simpler than the other
14:05:57 eskil: torarne: They are equally complicated and require people to read the docs to understand
14:05:58 torarne: eskil: on the other hand, these clients will still have to start dealing with the possibly of returning Qt::PrimaryOrienation initially
14:06:36 torarne: eskil: soltuion 1: A && B, two condition, soliution 2: A, one condition, how is 2 not simpler to both document and reason about?
14:07:13 torarne: eskil: the implementation is of course trivial in both cases
14:07:37 eskil: torarne: One solution is: "Default is PrimaryOrientation, orientation is updated when update mask is set". Second is: "Orientation is updated when update mask is set, otherwise it is PrimaryOrientation, even if update mask has once been set and then cleared again."
14:08:10 eskil: torarne: The complexity in both lies in the fact that the orientation property is updated from some API that needs to be turned on first
14:08:17 eskil: torarne: The second one is not simpler to my ears
14:09:29 eskil: torarne: I'd agree the second was simpler if there was some way for people to intuitively understand that "hey, it's returning primary orientation", that must mean I have to set a mask somewhere. But there really isn't. The only way to use this API is to read the docs first
14:10:25 andre_: VioletGiraffe: it's P2, and "open". could be worse. two common ways to increase attention are getting someone suitable to set the "Requested by support" label, or to link it the bug report to a suitable patch on codereview.
14:10:28 torarne: eskil: in that case I think it's better to leave the api as it is
14:10:42 eskil: torarne: Might be. What's the default now?
14:11:13 torarne: eskil: as it is today, it will init the orientation to whatever the plaform returns (typical startup orientation), or if the plaform returns Qt::PrimaryOrientation, it will resolve that and set it as the orienation
14:11:35 torarne: eskil: so effectivly it returns the result of primaryOrienation instead of Qt::PrimaryOrientation
14:11:53 torarne: unless the platform claims to know the orientation at startup by returning a concrete orientation
14:11:54 VioletGiraffe: andre_: thanks. I know it could be worse, and it's not a critical bug, but it does confine me quite a bit since I must use Qt4 until it's fixed. And I'm afraid the fix won't make it into Qt 5.4 which means another half a year wait.
14:12:30 eskil: torarne: Right. Which is not really usable for anything. I agree that it would be clearer if the default was always PrimaryOrientation unless it had actually been set, but it might not be worth changing that either
14:12:50 torarne: eskil: so Qscreen::orienation is effectly last-cached-or-known-orientation-or-primaryorientation-if-just-starting-up
14:13:13 eskil: torarne: Yeah, the default is currently undefined, which is not so nice
14:13:27 eskil: torarne: Then again, what would a user do with the default of that anyway?
14:16:10 torarne: hmm
14:19:04 torarne: eskil: even with the mask set, the return value may be "stale", as in the last seen value that matched the previous mask or no mask at all
14:25:06 eskil: torarne: It shouldnt have been a property at all, just a signal
14:26:45 eskil: torarne: with your patch it gets an undefined value between when the mask is set and the property is updated
14:32:26 torarne: eskil: hmm, you're right, good point
14:32:43 torarne: eskil: with either of "our" patches actually
14:32:59 torarne: eskil: which is why im leaning towards leaving it as "best-effort-or-cached/last"
14:35:09 eskil: torarne: yeah, i agree. and then find a better solution in qt 6 (..is going to be awesome)
14:35:13 torarne: haha