Revert "notification: Add platform-data and response to ActionInvoked signal"#1831
Revert "notification: Add platform-data and response to ActionInvoked signal"#18313v1n0 wants to merge 3 commits intoflatpak:mainfrom
Conversation
… signal" Sadly reusing the old activation API for v2 by adding more values to the parameters array was not working well enough for various reasons: - We used a DBus return value as a container for multiple (unrelated) and generic return values without being able to reuse any kind of type check provided by the platform. - Despite the API documented these return values as an ordered list of items with (ideally) a fixed position, it was not possible to maintain such promise in case of an empty parameter was provided by the app or - in future - an empty (!= no) response was sent. Note how `response` was referenced as the third parameter of such array, while this might have not been possible. - It was confusing for both applications using it and portal implementing the API, as it was not clear for the portals implementations what to pass back to the application when no parameter was received or what the application should expect to receive in such case as the first array value. So, let's go back to the v1 state, as this will allow us to provide a proper ActionInvoked2 implementation. This also implies breaking the `im.reply-with-text` category behavior though, since that's affected by the very same problem. We're doing this without changing the portal version though, as this won't break applications expecting a v2 portal anyways. This reverts commit 3aedb5f. This partially reverts commit def2ad7
…t response The text response is affected by the very same problem of ActionInvoked, but here it's even worse, since we rely on the `org.freedesktop.Application` API, and thus we can't modify it (unless we decide to add an ActivateAction2 method too). So, preserve the previous behavior, but explain what it should be done in case an application is not providing a target. We could have used a special parameter of the platform-data too, but it seems a bit out of scope there.
3v1n0
force-pushed
the
revert-notification-activation-v2-changes
branch
from
24db59b to
3db8c94
Compare
|
Yikes, The problem is that this should all work with So I really think we have to find a way to specify things with a |
What I'm saying is: can't we just send as the first and only parameter of the fdo apps Is there any restriction on sending them back the target as standalone value? |
|
I'm not sure if I understand. Do you propose to use the first element of Isn't that backwards incompatible? |
Yes
It is not, but it has not to be since with ActionInvoked2 we need to bump to v3 anyways |
Sure, but everything is supposed to work with If it were just about |
Yes, but we are not changing the signature of the method, we are changing its content and AFAIK, that's defined nowhere in the standard, but rather a portal decision to pass as first argument the So if an application is target the notification portal v3, then it has to expect the parameters to contain something else. It's a breaking change, yes. But How many applications really rely on the "target" anyways? And if they are choosing to use a new portal version they should be ready to adapt anyways. While the apps using libportal can just transparently be transitioned. If instead we don't want to break with the past we can just likely send to the app the Another option is instead to always use the |
True, but apps expect the action to be passed there already. You argue that if apps use notification portal v3, then they have to expect the the value to contain something else; but the code that handles I don't see how this could work.
That's a different argument, and I just don't know. @jsparber opinions? |
Well, it would just work in the same way it's already working, no? With GLib's commit Now, IMHO we could just switch to something else that is a bit more safer in this regards than returning a tuple of undefined length whose elements are defined by their index. However, if you want to be conservative, we can just do the same ourselves: if only a parameter (the target) is passed, we use the same logic as before, otherwise we return a vardict instead. And it would just be backward compatible for most scenarios. But honestly I'd prefer to avoid this kind of dualism (that again it is not new). |
|
Okay, I think I'm starting to understand. The app has to define the signature of the action anyway, so if the app a specific version of the notification portal, it has to expect the signature Yeah, okay, I'm convinced. |
Yep exactly... It's the app defining that, we just passed a value for now... I think the issue has actually created by
Good, now, how do we expose all this? I mean documentation-wise, while the |
|
We could maybe make it easier for us by having a new method to set the "used" version. Currently only the portal will tell the client its version, but what we need here is essentially the client opting-in a specific version, so we can just say that if the client opts-in to version 2, the Then it becomes rather easy because if we operate on a version 2 client, we always have a |
That's an option too, although I feel it's something that can be prone to races, unless we allow this to happen only as first method call ever, at the same time why an application should not know what version of the portal is currently running on? |
|
The problem I see is that the portal can't just change something in a new version because some clients will only support older versions. So you'd have to switch the signature based on if the notification contains something from version 2. That seems like a recipe for disaster, and people will get it wrong. We could maybe add a version field to the |
Well... Don't think it's a portal problem, if we document things properly TBH. Not to mention that "disaster" is very unlikely to happen as really I'm not aware of any implementation taking care of the "target" parameter.
That's maybe better... But, we may then have the other problem: applications not setting the version and thus they'll stick to version 1, that's probably not a big deal if they don't care about the new fields we send them back though. |
|
Could even make the dbus call fail if a v2 feature is being used without setting the version field to at least 2. That way you have to opt-in to v2, and then you have a specific signature no matter what else is in the vardict. |
2 participants
Sadly reusing the old activation API for v2 by adding more values to the
parameters array was not working well enough for various reasons:
and generic return values without being able to reuse any kind of type
check provided by the platform.
items with (ideally) a fixed position, it was not possible to
maintain such promise in case of an empty parameter was provided by
the app or - in future - an empty (!= no) response was sent.
Note how
responsewas referenced as the third parameter of sucharray, while this might have not been possible.
implementing the API, as it was not clear for the portals
implementations what to pass back to the application when no parameter
was received or what the application should expect to receive in such
case as the first array value.
So, let's go back to the v1 state, as this will allow us to provide a
proper ActionInvoked2 implementation.
This also implies breaking the
im.reply-with-textcategory behaviorthough, since that's affected by the very same problem.
We're doing this without changing the portal version though, as this
won't break applications expecting a v2 portal anyways.
This reverts commit 3aedb5f.
This partially reverts commit def2ad7
That said, this is not as easy as it should since
im.reply-with-textopens the gate to another problematic sitaution, reason why I think we should also modifyActionInvoked2to not send the parameters asavat all, but rather ass{av}and same for the exported actions: we just send them back ana{sv}withtargetandresponsefor now.