Don't start separate event loop for QFileDialog #16158
Conversation
glassez
added
GUI
|
tested - confirm, fixed. |
| torrent->setAutoTMMEnabled(false); | ||
| torrent->setSavePath(Utils::Fs::expandPathAbs(newLocation)); | ||
| } | ||
| const QVector<BitTorrent::Torrent *> torrents = getSelectedTorrents(); |
There was a problem hiding this comment.
Just for discussion (not a hard blocker):
Formally speaking this is the wrong approach (and this problem plague all GUI code). Is there a better way of solving it?
I suppose it can record a list of infohash instead and resolve it to torrent handle later when needed.
There was a problem hiding this comment.
Or we can have TorrentPtr like QPointer, which automatically becomes null when the torrent is removed?
There was a problem hiding this comment.
Formally speaking this is the wrong approach (and this problem plague all GUI code).
The real wrong case is passing QModelIndex (or Torrent *) list to asynchronous handler (since they can be invalidated in between).
I don't do this here.
There was a problem hiding this comment.
Or we can have TorrentPtr like QPointer, which automatically becomes null when the torrent is removed?
The idea is good, but how would the implementation look like? or is it even feasible?
The real wrong case is passing QModelIndex (or Torrent *) list to asynchronous handler (since they can be invalidated in between).
The invalidated entries will be discarded while the rest (valid) entries will still apply the action.
There was a problem hiding this comment.
The real wrong case is passing QModelIndex (or Torrent *) list to asynchronous handler (since they can be invalidated in between).
The invalidated entries will be discarded while the rest (valid) entries will still apply the action.
Seems you misunderstood me.
If you store pointers you can't know which of them are invalid. So you should either store Torrent IDs and then try to obtain corresponding torrents or store QPointers and check them for nullptr. Or obtain selected items in handler (as I'm done).
but how would the implementation look like?
Make Torrent to inherit QObject and store QPointers.
There was a problem hiding this comment.
If you store pointers you can't know which of them are invalid. So you should either store Torrent IDs and then try to obtain corresponding torrents or store QPointers and check them for nullptr. Or obtain selected items in handler (as I'm done).
That is why I said storing the infohash instead.
Make Torrent to inherit QObject and store QPointers.
That probably will do.
But obtaining selected items in handler seems more ergonomic to me (than store the list and then check its item).
AFAIK it might be possible that before the asynchronous handler took action the user reselected the torrents, even the timing window is very small this way (obtaining selected items in handler) is still wrong IMO.
There was a problem hiding this comment.
the user reselected the torrents, even the timing window is very small this way (obtaining selected items in handler)
Even if the user has time to click something at this time (during executing of discussed handler), this event (selecting/deselecting torrents) will be processed only after processing the current one.
There was a problem hiding this comment.
Even if the user has time to click something at this time (during executing of discussed handler), this event (selecting/deselecting torrents) will be processed only after processing the current one.
I don't think so, the handler is connected to QDialog::accepted and a lot of things can happen before the signal is triggered...
Although the dialog is made modal so users cannot really reselect torrents but I still don't consider this code robust as it isn't a general pattern for all dialogs (including non-modal ones).
There was a problem hiding this comment.
the handler is connected to
QDialog::acceptedand a lot of things can happen before the signal is triggered
Handlers (slots) are called directly unless "queued connection" is used.
Although the dialog is made
modalso users cannot really reselect torrents
Yes, it is.
it isn't a general pattern for all dialogs (including non-modal ones).
When you create some dialog that should affect some selection from other place non-modal so your intention is allowing the user to reconsider selection and apply dialog action to the final one (otherwise making dialog non-modal is meaningless/confusing). So it seems to be general pattern for all dialogs affecting some selection.
There was a problem hiding this comment.
Handlers (slots) are called directly unless "queued connection" is used.
I meant (generally speaking) a lot of UI events, background automatic processing, etc. could happen.
When you create some dialog that should affect some selection from other place non-modal so your intention is allowing the user to reconsider selection and apply dialog action to the final one (otherwise making dialog non-modal is meaningless/confusing). So it seems to be general pattern for all dialogs affecting some selection.
It would be correct only if the dialog says you can reselect the entries, which is not the case for most actions. This idea is not implied by dialogs (by default), which would be counter-intuitive.
otherwise making dialog non-modal is meaningless/confusing
Making dialog non-modal has other meanings, not just limited to this one.
It conflicts with QMenu on Qt6 that causes the crash.
|
re-tested with latest commit & all ok. |
It conflicts with QMenu on Qt6 that causes the crash. PR qbittorrent#16158.
It conflicts with QMenu on Qt6 that causes the crash. PR #16158.
It conflicts with QMenu on Qt6 that causes the crash.
Closes #15779.