[vlc-commits] [Git][videolan/vlc][master] 5 commits: qt: remove `QVLCDialog::keyPressEvent()`
Steve Lhomme (@robUx4)
gitlab at videolan.org
Fri May 2 11:09:14 UTC 2025
Steve Lhomme pushed to branch master at VideoLAN / VLC
Commits:
6a330a63 by Fatih Uzunoglu at 2025-05-02T10:47:43+00:00
qt: remove `QVLCDialog::keyPressEvent()`
- `QDialog` already handles this.
- The implementation is not correct here,
enter key should accept the dialog. It is
also not clear why a dialog specific `reject()`
is not called when encountering escape key.
- - - - -
95d0bc05 by Fatih Uzunoglu at 2025-05-02T10:47:43+00:00
qt: derive `PlaylistsDialog` from `QVLCDialog` instead of `QVLCFrame`
- - - - -
923aea96 by Fatih Uzunoglu at 2025-05-02T10:47:43+00:00
qt: make `PlaylistsDialog` no longer a singleton
Dialogs that carry the state of their initiators
should not be singleton. In this case, this dialog
always needs some sort of media to be functional.
This is already the case with the media info
dialog, for example, current media info dialog
is a singleton but media info dialog of a specific
media is not.
Since this dialog is always tied to some specific
media, I see no point of this being a singleton.
Its title suggests that this dialog is only meant
to be used for adding items to an existing or new
playlist.
Asynchronousity is also a problem when it is a
singleton, what happens if the user wants to add
items before the previous (asynchronous) addition
is not complete yet?
- - - - -
405e1efb by Fatih Uzunoglu at 2025-05-02T10:47:43+00:00
qt: when possible segregate playlists in playlist dialog
Currently the interface only provides "video" and "audio"
playlists. Mixed playlists don't seem to be presented.
This is something that can be handled trivially with drag
and drop case.
Adding to playlist action in context menu is more of a
problem, because in that case there is no specific open
playlist view that we can pick as the target.
I'm not sure what is the best behavior with regard to
the latter situation, but I assume we need to segregate
there too, because if the user can not make use of a
mixed playlist, there is no point creating them or
appending media.
I followed a naive approach, simply used `showPlayAsAudioAction`.
However, it could be possible to:
- If there is a single item, check if it is audio or video, and
pick the appropriate type playlist.
- If there are multiple items, always pick `PLAYLIST_TYPE_ALL`.
- If there are multiple items, iterate through all items, and
if all of them are the same, pick the appropriate playlist
(i.e. video, or audio playlist), if there are different type
of items, pick `PLAYLIST_TYPE_ALL`.
- - - - -
030af374 by Fatih Uzunoglu at 2025-05-02T10:47:43+00:00
qt: set transaction pending in `MLPlaylistListModel::create()`
- - - - -
16 changed files:
- modules/gui/qt/dialogs/dialogs_provider.cpp
- modules/gui/qt/dialogs/dialogs_provider.hpp
- modules/gui/qt/dialogs/gototime/gototime.cpp
- modules/gui/qt/dialogs/gototime/gototime.hpp
- modules/gui/qt/dialogs/open/openurl.hpp
- modules/gui/qt/dialogs/playlists/playlists.cpp
- modules/gui/qt/dialogs/playlists/playlists.hpp
- modules/gui/qt/dialogs/sout/convert.cpp
- modules/gui/qt/dialogs/sout/convert.hpp
- modules/gui/qt/dialogs/sout/profile_selector.cpp
- modules/gui/qt/dialogs/sout/profile_selector.hpp
- modules/gui/qt/medialibrary/mlplaylistlistmodel.cpp
- modules/gui/qt/medialibrary/qml/PlaylistMediaList.qml
- modules/gui/qt/util/qml/MLContextMenu.qml
- modules/gui/qt/widgets/native/qvlcframe.cpp
- modules/gui/qt/widgets/native/qvlcframe.hpp
Changes:
=====================================
modules/gui/qt/dialogs/dialogs_provider.cpp
=====================================
@@ -157,8 +157,8 @@ void DialogsProvider::customEvent( QEvent *event )
//FIXME
//playlistDialog(); break;
break;
- case INTF_DIALOG_PLAYLISTS:
- playlistsDialog(); break;
+ // case INTF_DIALOG_PLAYLISTS:
+ // FIXME: `intf_dialog_args_t` is not supported by `PlaylistsDialog`.
case INTF_DIALOG_MESSAGES:
messagesDialog(); break;
case INTF_DIALOG_FILEINFO:
@@ -411,21 +411,11 @@ void DialogsProvider::mediaCodecDialog()
m_mediaInfoDialog->hide();
}
-void DialogsProvider::playlistsDialog()
+void DialogsProvider::playlistsDialog( const QVariantList & medias, MLPlaylistListModel::PlaylistType type )
{
- toggleDialogVisible(m_playlistDialog);
-}
-
-void DialogsProvider::playlistsDialog( const QVariantList & medias )
-{
- ensureDialog(m_playlistDialog);
-
- m_playlistDialog->setMedias(medias);
-
- m_playlistDialog->show();
-
- // FIXME: We shouldn't have to call this on here.
- m_playlistDialog->activateWindow();
+ const auto dialog = new PlaylistsDialog(p_intf, medias, type);
+ dialog->setAttribute(Qt::WA_DeleteOnClose);
+ dialog->open();
}
void DialogsProvider::bookmarksDialog()
=====================================
modules/gui/qt/dialogs/dialogs_provider.hpp
=====================================
@@ -40,6 +40,7 @@
#include "util/shared_input_item.hpp"
#include "medialibrary/mlqmltypes.hpp"
+#include "medialibrary/mlplaylistlistmodel.hpp"
#include <QObject>
#include <QStringList>
@@ -148,7 +149,6 @@ private:
std::unique_ptr<HelpDialog> m_helpDialog;
std::unique_ptr<AboutDialog> m_aboutDialog;
std::unique_ptr<MediaInfoDialog> m_mediaInfoDialog;
- std::unique_ptr<PlaylistsDialog> m_playlistDialog;
std::unique_ptr<BookmarksDialog> m_bookmarkDialog;
std::unique_ptr<PodcastConfigDialog> m_podcastDialog;
std::unique_ptr<PluginDialog> m_pluginDialog;
@@ -170,8 +170,7 @@ private:
void toggleDialogVisible(std::unique_ptr<T>& dialog);
public slots:
- void playlistsDialog();
- void playlistsDialog( const QVariantList & listMedia );
+ void playlistsDialog( const QVariantList & listMedia, MLPlaylistListModel::PlaylistType type = MLPlaylistListModel::PLAYLIST_TYPE_ALL);
void bookmarksDialog();
void mediaInfoDialog( void );
void mediaInfoDialog( const SharedInputItem& inputItem );
=====================================
modules/gui/qt/dialogs/gototime/gototime.cpp
=====================================
@@ -70,8 +70,9 @@ GotoTimeDialog::GotoTimeDialog( qt_intf_t *_p_intf)
mainLayout->addWidget( buttonBox, 1, 0, 1, 3 );
- BUTTONACT( gotoButton, &GotoTimeDialog::close );
- BUTTONACT( cancelButton, &GotoTimeDialog::cancel );
+ connect( buttonBox, &QDialogButtonBox::accepted, this, &GotoTimeDialog::accept );
+ connect( buttonBox, &QDialogButtonBox::rejected, this, &GotoTimeDialog::reject );
+
BUTTONACT( resetButton, &GotoTimeDialog::reset );
QVLCTools::restoreWidgetPosition( p_intf, "gototimedialog", this );
@@ -95,20 +96,21 @@ void GotoTimeDialog::toggleVisible()
activateWindow();
}
-void GotoTimeDialog::cancel()
+void GotoTimeDialog::reject()
{
reset();
- toggleVisible();
+ QVLCDialog::reject();
}
-void GotoTimeDialog::close()
+void GotoTimeDialog::accept()
{
if ( THEMIM->hasInput() )
{
int i_time = QTime( 0, 0, 0 ).msecsTo( timeEdit->time() );
THEMIM->setTime( VLC_TICK_FROM_MS(i_time) );
}
- toggleVisible();
+
+ QVLCDialog::accept();
}
void GotoTimeDialog::reset()
=====================================
modules/gui/qt/dialogs/gototime/gototime.hpp
=====================================
@@ -36,9 +36,11 @@ public:
void toggleVisible();
+public slots:
+ void accept() override;
+ void reject() override;
+
private slots:
- void close() override;
- void cancel() override;
void reset();
private:
=====================================
modules/gui/qt/dialogs/open/openurl.hpp
=====================================
@@ -55,7 +55,7 @@ public:
void showEvent( QShowEvent *ev ) override;
public slots:
- void close() override { play(); }
+ void accept() override { play(); QVLCDialog::accept(); }
};
=====================================
modules/gui/qt/dialogs/playlists/playlists.cpp
=====================================
@@ -45,14 +45,13 @@
// Ctor / dtor
//-------------------------------------------------------------------------------------------------
-PlaylistsDialog::PlaylistsDialog(qt_intf_t * _p_intf) : QVLCFrame(_p_intf)
+PlaylistsDialog::PlaylistsDialog(qt_intf_t * _p_intf, const QVariantList &media, MLPlaylistListModel::PlaylistType type, QWindow *parent)
+ : QVLCDialog(parent, _p_intf)
{
MainCtx * mainCtx = p_intf->p_mi;
assert(mainCtx->hasMediaLibrary());
- setWindowFlags(Qt::Tool);
-
setWindowRole("vlc-playlists");
setWindowTitle(qtr("Add to Playlist"));
@@ -70,6 +69,7 @@ PlaylistsDialog::PlaylistsDialog(qt_intf_t * _p_intf) : QVLCFrame(_p_intf)
connect(m_playlists, &QTreeView::doubleClicked, this, &PlaylistsDialog::onDoubleClicked);
m_model = new MLPlaylistListModel(m_playlists);
+ m_model->setPlaylistType(type);
m_model->setMl(mainCtx->getMediaLibrary());
@@ -107,8 +107,8 @@ PlaylistsDialog::PlaylistsDialog(qt_intf_t * _p_intf) : QVLCFrame(_p_intf)
m_button->setEnabled(false);
- connect(buttonBox, &QDialogButtonBox::accepted, this, &PlaylistsDialog::onAccepted);
- connect(buttonBox, &QDialogButtonBox::rejected, this, &PlaylistsDialog::close);
+ connect(buttonBox, &QDialogButtonBox::accepted, this, &PlaylistsDialog::accept);
+ connect(buttonBox, &QDialogButtonBox::rejected, this, &PlaylistsDialog::reject);
m_label = new QLabel(this);
@@ -126,52 +126,20 @@ PlaylistsDialog::PlaylistsDialog(qt_intf_t * _p_intf) : QVLCFrame(_p_intf)
layout->addLayout(layoutButtons);
// FIXME: Is this the right default size ?
- restoreWidgetPosition("Playlists", QSize(320, 320));
+ QVLCTools::restoreWidgetPosition(p_intf, "Playlists", this, QSize(320, 320));
updateGeometry();
-}
-
-PlaylistsDialog::~PlaylistsDialog() /* override */
-{
- saveWidgetPosition("Playlists");
-}
-
-//-------------------------------------------------------------------------------------------------
-// Interface
-//-------------------------------------------------------------------------------------------------
-/* Q_INVOKABLE */ void PlaylistsDialog::setMedias(const QVariantList & medias)
-{
- m_ids = medias;
-
- m_label->setText(qtr("%1 Media(s)").arg(m_ids.count()));
-}
-
-//-------------------------------------------------------------------------------------------------
-// Events
-//-------------------------------------------------------------------------------------------------
-
-void PlaylistsDialog::hideEvent(QHideEvent *) /* override */
-{
- // NOTE: We clear the lineEdit when hiding the dialog.
- m_lineEdit->clear();
-}
-
-void PlaylistsDialog::keyPressEvent(QKeyEvent * event) /* override */
-{
- int key = event->key();
-
- // FIXME: For some reason we have to do this manually on here. Shouldn't QDialogButtonBox do
- // this automatically ?
- if (key == Qt::Key_Return || key == Qt::Key_Enter)
{
- if (m_button->isEnabled())
- onAccepted();
-
- return;
+ assert(!media.isEmpty());
+ m_ids = media;
+ m_label->setText(qtr("%1 Media").arg(m_ids.count()));
}
+}
- QVLCFrame::keyPressEvent(event);
+PlaylistsDialog::~PlaylistsDialog()
+{
+ QVLCTools::saveWidgetPosition(p_intf, "Playlists", this);
}
//-------------------------------------------------------------------------------------------------
@@ -191,7 +159,7 @@ void PlaylistsDialog::onDoubleClicked()
if (m_button->isEnabled() == false)
return;
- onAccepted();
+ accept();
}
//-------------------------------------------------------------------------------------------------
@@ -207,7 +175,7 @@ void PlaylistsDialog::onTextEdited()
m_button->setEnabled(true);
}
-void PlaylistsDialog::onAccepted()
+void PlaylistsDialog::accept()
{
QString text = m_lineEdit->text();
@@ -222,5 +190,18 @@ void PlaylistsDialog::onAccepted()
m_model->create(text, m_ids);
}
- close();
+ // Model is asynchronous, we need to wait for the model to complete processing...
+ if (Q_LIKELY(m_model->transactionPending())) // Same thread, TOCTOU is not relevant.
+ {
+ hide();
+
+ connect(m_model, &MLPlaylistListModel::transactionPendingChanged, this, [this](bool done) {
+ if (!done)
+ QVLCDialog::accept();
+ }, Qt::SingleShotConnection);
+ }
+ else
+ {
+ QVLCDialog::accept();
+ }
}
=====================================
modules/gui/qt/dialogs/playlists/playlists.hpp
=====================================
@@ -24,31 +24,29 @@
#define QVLC_PLAYLISTS_H_ 1
// VLC includes
+#include "medialibrary/mlplaylistlistmodel.hpp"
#include <widgets/native/qvlcframe.hpp>
// Forward declarations
-class MLPlaylistListModel;
class QTreeView;
class QLineEdit;
class QLabel;
class QPushButton;
-class PlaylistsDialog : public QVLCFrame
+class PlaylistsDialog : public QVLCDialog
{
Q_OBJECT
public: // Ctor / dtor
- PlaylistsDialog(qt_intf_t *);
+ explicit PlaylistsDialog(qt_intf_t *_p_intf,
+ const QVariantList & media,
+ MLPlaylistListModel::PlaylistType type = MLPlaylistListModel::PLAYLIST_TYPE_ALL,
+ QWindow *parent = nullptr);
- ~PlaylistsDialog() override;
+ ~PlaylistsDialog();
-public: // Interface
- Q_INVOKABLE void setMedias(const QVariantList & medias);
-
-protected: // Events
- void hideEvent(QHideEvent * event) override;
-
- void keyPressEvent(QKeyEvent * event) override;
+public slots:
+ void accept() override;
private slots:
void onClicked ();
@@ -56,8 +54,6 @@ private slots:
void onTextEdited();
- void onAccepted();
-
private:
QVariantList m_ids;
=====================================
modules/gui/qt/dialogs/sout/convert.cpp
=====================================
@@ -143,8 +143,8 @@ ConvertDialog::ConvertDialog( QWindow *parent, qt_intf_t *_p_intf,
mainLayout->addWidget( buttonBox, 5, 3 );
- BUTTONACT( okButton, &ConvertDialog::close );
- BUTTONACT( cancelButton, &ConvertDialog::cancel );
+ connect( buttonBox, &QDialogButtonBox::accepted, this, &ConvertDialog::accept );
+ connect( buttonBox, &QDialogButtonBox::rejected, this, &ConvertDialog::reject );
connect( convertRadio, &QRadioButton::toggled, convertPanel, &QWidget::setEnabled );
connect( profile, &VLCProfileSelector::optionsChanged, this, &ConvertDialog::setDestinationFileExtension );
@@ -166,12 +166,7 @@ void ConvertDialog::fileBrowse()
setDestinationFileExtension();
}
-void ConvertDialog::cancel()
-{
- reject();
-}
-
-void ConvertDialog::close()
+void ConvertDialog::accept()
{
hide();
@@ -250,7 +245,8 @@ void ConvertDialog::close()
msg_Dbg( p_intf, "Transcode chain: %s", qtu( mrl.to_string() ) );
mrls.append(mrl.to_string());
}
- accept();
+
+ QVLCDialog::accept();
}
void ConvertDialog::setDestinationFileExtension()
=====================================
modules/gui/qt/dialogs/sout/convert.hpp
=====================================
@@ -55,9 +55,10 @@ private:
QUrl outgoingMRL;
QStringList mrls;
+public slots:
+ void accept() override;
+
private slots:
- void close() override;
- void cancel() override;
void fileBrowse();
void setDestinationFileExtension();
void validate();
=====================================
modules/gui/qt/dialogs/sout/profile_selector.cpp
=====================================
@@ -443,10 +443,10 @@ VLCProfileEditor::VLCProfileEditor( const QString& qs_name, const QString& value
QPushButton *saveButton = new QPushButton(
( qs_name.isEmpty() ) ? qtr( "Create" ) : qtr( "Save" ) );
ui.buttonBox->addButton( saveButton, QDialogButtonBox::AcceptRole );
- BUTTONACT( saveButton, &VLCProfileEditor::close );
+ connect( ui.buttonBox, &QDialogButtonBox::accepted, this, &VLCProfileEditor::accept );
QPushButton *cancelButton = new QPushButton( qtr( "Cancel" ) );
ui.buttonBox->addButton( cancelButton, QDialogButtonBox::RejectRole );
- BUTTONACT( cancelButton, &VLCProfileEditor::reject );
+ connect( ui.buttonBox, &QDialogButtonBox::rejected, this, &VLCProfileEditor::reject );
connect( ui.valueholder_video_copy, &QtCheckboxChanged,
this, &VLCProfileEditor::activatePanels );
@@ -776,7 +776,7 @@ void VLCProfileEditor::fillProfileOldFormat( const QString& qs )
ui.valueholder_subtitles_overlay->setChecked( options[15].toInt() );
}
-void VLCProfileEditor::close()
+void VLCProfileEditor::accept()
{
if( ui.profileLine->text().isEmpty() )
{
@@ -787,7 +787,7 @@ void VLCProfileEditor::close()
}
name = ui.profileLine->text();
- accept();
+ QVLCDialog::accept();
}
#define currentData( box ) box->itemData( box->currentIndex() )
=====================================
modules/gui/qt/dialogs/sout/profile_selector.hpp
=====================================
@@ -84,8 +84,8 @@ private:
QHash<QString, resultset> caps;
void loadCapabilities();
void reset();
-protected slots:
- void close() override;
+public slots:
+ void accept() override;
private slots:
void muxSelected();
void codecSelected();
=====================================
modules/gui/qt/medialibrary/mlplaylistlistmodel.cpp
=====================================
@@ -92,6 +92,8 @@ void appendMediaIntoPlaylist(vlc_medialibrary_t* ml, int64_t playlistId, const s
MLItemId createdPlaylistId;
};
+ setTransactionPending(true);
+
m_mediaLib->runOnMLThread<Ctx>(this,
//ML thread
[name](vlc_medialibrary_t* ml, Ctx& ctx)
@@ -104,6 +106,8 @@ void appendMediaIntoPlaylist(vlc_medialibrary_t* ml, int64_t playlistId, const s
}
},
[this, initialItems](quint64, const Ctx& ctx) {
+ endTransaction(); // this is intentionally called here and not after `append()`
+
if (ctx.createdPlaylistId.id)
{
append(ctx.createdPlaylistId, initialItems);
=====================================
modules/gui/qt/medialibrary/qml/PlaylistMediaList.qml
=====================================
@@ -158,7 +158,7 @@ MainViewLoader {
drop.accepted = true
return item.getSelectedInputItem().then(inputItems => {
if (index === undefined)
- DialogsProvider.playlistsDialog(inputItems)
+ DialogsProvider.playlistsDialog(inputItems, (root.model as MLPlaylistListModel).playlistType)
else
root.model.append(root.model.getItemId(index), inputItems)
})
@@ -168,7 +168,7 @@ MainViewLoader {
for (let url in drop.urls)
urlList.push(drop.urls[url])
if (index === undefined)
- DialogsProvider.playlistsDialog(inputItems)
+ DialogsProvider.playlistsDialog(inputItems, (root.model as MLPlaylistListModel).playlistType)
else
root.model.append(root.model.getItemId(index), urlList)
} else {
=====================================
modules/gui/qt/util/qml/MLContextMenu.qml
=====================================
@@ -124,7 +124,14 @@ NativeMenu {
}
function addToAPlaylist(dataList, options, indexes) {
- DialogsProvider.playlistsDialog(_mlIDList(dataList))
+ let type
+ // if (dataList.length === 1)
+ type = root.showPlayAsAudioAction ? MLPlaylistListModel.PLAYLIST_TYPE_VIDEO
+ : MLPlaylistListModel.PLAYLIST_TYPE_AUDIO
+ // else
+ // type = MLPlaylistListModel.PLAYLIST_TYPE_ALL // iterate through items?
+
+ DialogsProvider.playlistsDialog(_mlIDList(dataList), type)
}
function addFavorite(dataList, options, indexes) {
=====================================
modules/gui/qt/widgets/native/qvlcframe.cpp
=====================================
@@ -220,19 +220,6 @@ void QVLCFrame::keyPressEvent(QKeyEvent *keyEvent)
}
}
-void QVLCDialog::keyPressEvent(QKeyEvent *keyEvent)
-{
- if (keyEvent->key() == Qt::Key_Escape)
- {
- this->cancel();
- }
- else if (keyEvent->key() == Qt::Key_Return ||
- keyEvent->key() == Qt::Key_Enter)
- {
- this->close();
- }
-}
-
void QVLCDialog::setWindowTransientParent(QWidget* widget, QWindow* parent, qt_intf_t* p_intf)
{
assert(widget);
=====================================
modules/gui/qt/widgets/native/qvlcframe.hpp
=====================================
@@ -121,16 +121,6 @@ public:
protected:
qt_intf_t *p_intf;
-
- virtual void cancel()
- {
- hide();
- }
- virtual void close()
- {
- hide();
- }
- void keyPressEvent( QKeyEvent *keyEvent ) override;
};
#endif
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/a259a9abbdc5ce5f07c10afd5787db65e3c122a6...030af374bc9f59395f7265d5aded67a3a27706b8
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/a259a9abbdc5ce5f07c10afd5787db65e3c122a6...030af374bc9f59395f7265d5aded67a3a27706b8
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list