Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't start separate event loop for QFileDialog #16158

Merged
merged 1 commit into from
Jan 20, 2022
Merged

Don't start separate event loop for QFileDialog #16158

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/gui/transferlistwidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,19 +331,35 @@ QVector<BitTorrent::Torrent *> TransferListWidget::getVisibleTorrents() const
void TransferListWidget::setSelectedTorrentsLocation()
{
const QVector<BitTorrent::Torrent *> torrents = getSelectedTorrents();
if (torrents.isEmpty()) return;
if (torrents.isEmpty())
return;

const QString oldLocation = torrents[0]->savePath();
const QString newLocation = QFileDialog::getExistingDirectory(this, tr("Choose save path"), oldLocation,
(QFileDialog::DontConfirmOverwrite | QFileDialog::ShowDirsOnly | QFileDialog::HideNameFilterDetails));
if (newLocation.isEmpty() || !QDir(newLocation).exists()) return;

// Actually move storage
for (BitTorrent::Torrent *const torrent : torrents)
auto fileDialog = new QFileDialog(this, tr("Choose save path"), oldLocation);
fileDialog->setAttribute(Qt::WA_DeleteOnClose);
fileDialog->setFileMode(QFileDialog::Directory);
fileDialog->setModal(true);
fileDialog->setOptions(QFileDialog::DontConfirmOverwrite | QFileDialog::ShowDirsOnly | QFileDialog::HideNameFilterDetails);
connect(fileDialog, &QDialog::accepted, this, [this, fileDialog]()
{
torrent->setAutoTMMEnabled(false);
torrent->setSavePath(Utils::Fs::expandPathAbs(newLocation));
}
const QVector<BitTorrent::Torrent *> torrents = getSelectedTorrents();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can have TorrentPtr like QPointer, which automatically becomes null when the torrent is removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Chocobo1 Chocobo1 Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@glassez glassez Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Chocobo1 Chocobo1 Jan 19, 2022 •
edited by glassez
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the handler is connected to QDialog::accepted and 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 modal so 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.

Copy link
Member

@Chocobo1 Chocobo1 Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (torrents.isEmpty())
return;

const QString newLocation = fileDialog->selectedFiles().constFirst();
if (newLocation.isEmpty() || !QDir(newLocation).exists())
return;

// Actually move storage
for (BitTorrent::Torrent *const torrent : torrents)
{
torrent->setAutoTMMEnabled(false);
torrent->setSavePath(newLocation);
}
});

fileDialog->show();
}

void TransferListWidget::pauseAllTorrents()
Expand Down