Conversation
danirabbit
commented
Feb 13, 2026
danirabbit
commented
Co-authored-by: Ryo Nakano <ryonakaknock3@gmail.com>
ryonakano
left a comment
ryonakano
left a comment
There was a problem hiding this comment.
A few other comments and questions.
|
|
||
| allow_button.clicked.connect (() => response (ResponseType.ALLOW)); | ||
| cancel_button.clicked.connect (() => response (ResponseType.CANCEL)); | ||
| close_request.connect (() => { response (ResponseType.CANCEL); }); |
There was a problem hiding this comment.
Is CANCEL instead of DELETE_EVENT here expected?
There was a problem hiding this comment.
That's a great question. I just copied what was already being done in AccessDialog. There's no close button and the escape key doesn't close the dialog, but you can secondary click on the header to close from the context menu. So I'm guessing this was done so that we fail safely in that case and make sure permission isn't granted unless an explicit choice is made
There was a problem hiding this comment.
Hmm, doesn't calling response (ResponseType.DELETE_EVENT) return the caller of PortalDialog and Portal classes (i.e. apps that requested access) that its access request was rejected then? 🤔 Also Access.Dialog.on_close() calls response (DELETE_EVENT); instead of response (CANCEL);.
We should leave a comment why we use CANCEL instead of DELETE_EVENT if your guess is true. Otherwise I think we should use DELETE_EVENT.
I just copied what was already being done in AccessDialog.
Could you tell me where is AccessDialog.vala? (Gala or Greeter?)
There was a problem hiding this comment.
Sorry I meant I was trying to just adapt from here:
portals/src/Access/Dialog.vala
Line 103 in 851f29d
But yeah I don't have any strong feelings about this and I didn't put a lot of time into understanding it. Happy to remove this if it doesn't make sense!
2 participants
