<div dir="ltr"><div><div><div>Hi!<br><br></div>Could you give me some help/feedback, please? (see my previous email)<br></div>I'd love to be able to finish this feature!<br><br></div>Have a nice day!<br></div><div class="gmail_extra">
<br><br><div class="gmail_quote">2014-07-24 11:49 GMT+02:00 David Sferruzza <span dir="ltr"><<a href="mailto:david.sferruzza@gmail.com" target="_blank">david.sferruzza@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div><div><div>Hi!<br><br></div>I just saw your answer (didn't receive it by email... weird): <a href="https://mailman.videolan.org/pipermail/vlc-devel/2014-July/098928.html" target="_blank">https://mailman.videolan.org/pipermail/vlc-devel/2014-July/098928.html</a><br>

</div><div>I understand the race condition issue.<br></div><div><br></div><div>So the problem is in playlist/thread.c: when the NextItem() function is called, the value of the state can change between the var_GetBool() and the  var_SetBool().<br>

</div><div>In term of user experience, this would result in the following situation: if the user uncheck "Stop after current track" at the exact moment NextItem() is executing, VLC could ignore it and stop anyway.<br>

</div><div>If I'm right (am I?) this doesn't feel a really bad behavior as it would happen very rarely and wouldn't be really annoying for the user.<br><br></div><div>Anyway, I tried to fix it, but I got stuck.<br>

</div><div>If I use var_GetAndSet(), the only i_action value I can use when manipulating a boolean would be <em>VLC_VAR_BOOL_TOGGLE</em>, according to <a href="https://www.videolan.org/developers/vlc/doc/doxygen/html/group__var__GetAndSet.html" target="_blank">https://www.videolan.org/developers/vlc/doc/doxygen/html/group__var__GetAndSet.html</a>.<br>

</div><div>But that's not the behavior I want (?).<br></div><div>I want something like: if (var is false) then do nothing ; but if (var is true) then set it to false immediately and do some actions.<br></div><div>Does this make sense?<br>

</div><div><br></div><div>Also, I can't find any reference (in the Doxygen doc, and in the codebase) to the var_ExchangeBool() function you mentioned.<br></div><div>Could you give me a hint on that?<br><br></div><div>

Apart from that, is the rest of my patch acceptable?<br><br></div><div>Have a nice day!<br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-16 18:18 GMT+02:00 David Sferruzza <span dir="ltr"><<a href="mailto:david.sferruzza@gmail.com" target="_blank">david.sferruzza@gmail.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div>Hi!<br><br></div>Thanks for your advice!<br></div>I started over: there are 2 diffs replacing this one at the end of this email.<br>

</div><div>(If necessary, I will start a new thread when the patch is nice enough.)<br>
</div><div><br></div><div>I put the logic into the playlist.<br></div><div>But I didn't get what you meant by "That's a bit racy. I would add an exchange operation to var_GetAndSet(). ...then you could do: else if( var_ExchangeBool( p_playlist, "play-and-stop-once", false ) )"<br>


</div><div>I didn't find what var_ExchangeBool was supposed to do (I searched here: <a href="https://wiki.videolan.org/Hacker_Guide/Variables/" target="_blank">https://wiki.videolan.org/Hacker_Guide/Variables/</a> and here: <a href="https://www.videolan.org/developers/vlc/doc/doxygen/html/group__variables.html" target="_blank">https://www.videolan.org/developers/vlc/doc/doxygen/html/group__variables.html</a> and also in the codebase itself).<br>


<br></div><div>The following diffs seem to work.<br></div><div>What do you think?<br></div><div><br></div>Playlist:<br><br>diff --git a/src/playlist/control.c b/src/playlist/control.c<br>index e1be944..3ac7ac8 100644<br>

--- a/src/playlist/control.c<br>
+++ b/src/playlist/control.c<br>@@ -84,6 +84,7 @@ static int PlaylistVAControl( playlist_t * p_playlist, int i_query, va_list args<br>         pl_priv(p_playlist)->request.i_status = PLAYLIST_STOPPED;<br>         pl_priv(p_playlist)->request.b_request = true;<br>


         pl_priv(p_playlist)->request.p_item = NULL;<div><br>+        var_SetBool( p_playlist, "play-and-stop-once", false );<br></div>         break;<br> <br>     // Node can be null, it will keep the same. Use with care ...<br>


diff --git a/src/playlist/engine.c b/src/playlist/engine.c<br>index ebb46dc..77ea295 100644<br>--- a/src/playlist/engine.c<br>+++ b/src/playlist/engine.c<br>@@ -275,6 +275,9 @@ playlist_t *playlist_Create( vlc_object_t *p_parent )<br>


     pl_priv(p_playlist)->request.b_request = false;<br>     pl_priv(p_playlist)->status.i_status = PLAYLIST_STOPPED;<br> <br>+    var_Create( p_playlist, "play-and-stop-once", VLC_VAR_BOOL );<div>
<br>+    var_SetBool( p_playlist, "play-and-stop-once", false );<br></div>
+<br>     if(b_ml)<br>         playlist_MLLoad( p_playlist );<br> <br>diff --git a/src/playlist/thread.c b/src/playlist/thread.c<br>index a6ecbdb..4f2efb6 100644<div><br>--- a/src/playlist/thread.c<br>+++ b/src/playlist/thread.c<br>


@@ -359,6 +359,7 @@ static playlist_item_t *NextItem( playlist_t *p_playlist )<br>         bool b_loop = var_GetBool( p_playlist, "loop" );<br>         bool b_repeat = var_GetBool( p_playlist, "repeat" );<br>


         bool b_playstop = var_InheritBool( p_playlist, "play-and-stop" );<br></div>+        bool b_playstoponce = var_GetBool( p_playlist, "play-and-stop-once" );<div><br> <br>         /* Repeat and play/stop */<br>


         if( b_repeat && get_current_status_item( p_playlist ) )<br>@@ -371,6 +372,12 @@ static playlist_item_t *NextItem( playlist_t *p_playlist )<br>             msg_Dbg( p_playlist,"stopping (play and stop)" );<br>


             return NULL;<br>         }<br>+        else if( b_playstoponce )<br>+        {<br>+            msg_Dbg( p_playlist, "stopping (play and stop once)" );<br>+            var_SetBool( p_playlist, "play-and-stop-once", false );<br>

</div><div>
+            return NULL;<br>+        }<br> <br>         /* */<br>         if( get_current_status_item( p_playlist ) )<br><br></div></div>Qt:<br><br>diff --git a/modules/gui/qt4/input_manager.cpp b/modules/gui/qt4/input_manager.cpp<br>


index f72b3b3..5c3995d 100644<br>--- a/modules/gui/qt4/input_manager.cpp<br>+++ b/modules/gui/qt4/input_manager.cpp<br>@@ -1205,6 +1205,16 @@ bool MainInputManager::getPlayExitState()<div><br>     return var_InheritBool( THEPL, "play-and-exit" );<br>


 }<br> <br>+void MainInputManager::activatePlayStopOnce( bool b_exit )<br>+{<br></div>+    var_SetBool( THEPL, "play-and-stop-once", b_exit );<div><div><br>+}<br>+<br>+bool MainInputManager::getPlayStopOnceState()<br>

+{<br>+    return var_InheritBool( THEPL, "play-and-stop-once" );<br>
+}<br>+<br> bool MainInputManager::hasEmptyPlaylist()<br> {<br>     playlist_Lock( THEPL );<br>diff --git a/modules/gui/qt4/input_manager.hpp b/modules/gui/qt4/input_manager.hpp<br>index dac3bc3..6932a77 100644<br>--- a/modules/gui/qt4/input_manager.hpp<br>


+++ b/modules/gui/qt4/input_manager.hpp<br>@@ -268,6 +268,7 @@ public:<br>     audio_output_t *getAout();<br> <br>     bool getPlayExitState();<br>+    bool getPlayStopOnceState();<br>     bool hasEmptyPlaylist();<br> <br>


     void requestVoutUpdate() { return im->UpdateVout(); }<br>@@ -298,6 +299,7 @@ public slots:<br>     void prev();<br>     void prevOrReset();<br>     void activatePlayQuit( bool );<br>+    void activatePlayStopOnce( bool );<br>


 <br>     void loopRepeatLoopStatus();<br> <br>diff --git a/modules/gui/qt4/menus.cpp b/modules/gui/qt4/menus.cpp<br>index a29333b..584cc6e 100644<br>--- a/modules/gui/qt4/menus.cpp<br>+++ b/modules/gui/qt4/menus.cpp<br>

@@ -826,6 +826,15 @@ void VLCMenuBar::PopupMenuPlaylistEntries( QMenu *menu,<br>
         action->setEnabled( false );<br>     action->setData( ACTION_DELETE_ON_REBUILD );<br> <br>+    /* Stop after current track */<br>+    action = addMIMStaticEntry( p_intf, menu, qtr( "Stop &after current track" ),<br>


+            "", SLOT( activatePlayStopOnce( bool ) ), true );<br>+    action->setCheckable( true );<br>+    action->setChecked( THEMIM->getPlayStopOnceState() );<br>+    if( !p_input )<br>+        action->setEnabled( false );<br>


+    action->setData( ACTION_DELETE_ON_REBUILD );<br>+<br>     /* Next / Previous */<br>     bool bPlaylistEmpty = THEMIM->hasEmptyPlaylist();<br>     action = addMIMStaticEntry( p_intf, menu, qtr( "Pre&vious" ),<br>


</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-15 10:36 GMT+02:00 Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net" target="_blank">remi@remlab.net</a>></span>:<div>

<div><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Le 2014-07-15 11:08, David Sferruzza a écrit :<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is a patch for #10732<br>
Work document: <a href="https://gist.github.com/dsferruzza/8e9079a329975f3579e9" target="_blank">https://gist.github.com/<u></u>dsferruzza/<u></u>8e9079a329975f3579e9</a><br>
---<br>
 modules/gui/qt4/input_manager.<u></u>cpp | 12 ++++++++++++<br>
 modules/gui/qt4/input_manager.<u></u>hpp |  2 ++<br>
 modules/gui/qt4/menus.cpp         |  9 +++++++++<br>
 src/libvlc-module.c               |  6 ++++++<br>
 src/playlist/thread.c             |  7 +++++++<br>
 5 files changed, 36 insertions(+)<br>
<br>
diff --git a/modules/gui/qt4/input_<u></u>manager.cpp<br>
b/modules/gui/qt4/input_<u></u>manager.cpp<br>
index f72b3b3..d69233a 100644<br>
--- a/modules/gui/qt4/input_<u></u>manager.cpp<br>
+++ b/modules/gui/qt4/input_<u></u>manager.cpp<br>
@@ -1097,6 +1097,7 @@ void MainInputManager::customEvent( QEvent *event )<br>
 void MainInputManager::stop()<br>
 {<br>
    playlist_Stop( THEPL );<br>
+   var_SetBool( THEPL, "play-and-stop-once", false );<br>
</blockquote>
<br></div>
IMHO, this belongs in the playlist state machine, not the UI.<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 }<br>
<br>
 void MainInputManager::next()<br>
@@ -1205,6 +1206,17 @@ bool MainInputManager::<u></u>getPlayExitState()<br>
     return var_InheritBool( THEPL, "play-and-exit" );<br>
 }<br>
<br>
+void MainInputManager::<u></u>activatePlayStopOnce( bool b_exit )<br>
+{<br>
+    var_Create( THEPL, "play-and-stop-once", VLC_VAR_BOOL );<br>
</blockquote>
<br></div>
Ditto for var_Create().<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    var_SetBool( THEPL, "play-and-stop-once", b_exit );<br>
+}<br>
+<br>
+bool MainInputManager::<u></u>getPlayStopOnceState()<br>
+{<br>
+    return var_InheritBool( THEPL, "play-and-stop-once" );<br>
+}<br>
+<br>
 bool MainInputManager::<u></u>hasEmptyPlaylist()<br>
 {<br>
     playlist_Lock( THEPL );<br>
diff --git a/modules/gui/qt4/input_<u></u>manager.hpp<br>
b/modules/gui/qt4/input_<u></u>manager.hpp<br>
index dac3bc3..6932a77 100644<br>
--- a/modules/gui/qt4/input_<u></u>manager.hpp<br>
+++ b/modules/gui/qt4/input_<u></u>manager.hpp<br>
@@ -268,6 +268,7 @@ public:<br>
     audio_output_t *getAout();<br>
<br>
     bool getPlayExitState();<br>
+    bool getPlayStopOnceState();<br>
     bool hasEmptyPlaylist();<br>
<br>
     void requestVoutUpdate() { return im->UpdateVout(); }<br>
@@ -298,6 +299,7 @@ public slots:<br>
     void prev();<br>
     void prevOrReset();<br>
     void activatePlayQuit( bool );<br>
+    void activatePlayStopOnce( bool );<br>
<br>
     void loopRepeatLoopStatus();<br>
<br>
diff --git a/modules/gui/qt4/menus.cpp b/modules/gui/qt4/menus.cpp<br>
index a29333b..584cc6e 100644<br>
--- a/modules/gui/qt4/menus.cpp<br>
+++ b/modules/gui/qt4/menus.cpp<br>
@@ -826,6 +826,15 @@ void VLCMenuBar::<u></u>PopupMenuPlaylistEntries( QMenu *menu,<br>
         action->setEnabled( false );<br>
     action->setData( ACTION_DELETE_ON_REBUILD );<br>
<br>
+    /* Stop after current track */<br>
+    action = addMIMStaticEntry( p_intf, menu, qtr( "Stop &after<br>
current track" ),<br>
+            "", SLOT( activatePlayStopOnce( bool ) ), true );<br>
+    action->setCheckable( true );<br>
+    action->setChecked( THEMIM->getPlayStopOnceState() );<br>
+    if( !p_input )<br>
+        action->setEnabled( false );<br>
+    action->setData( ACTION_DELETE_ON_REBUILD );<br>
+<br>
     /* Next / Previous */<br>
     bool bPlaylistEmpty = THEMIM->hasEmptyPlaylist();<br>
     action = addMIMStaticEntry( p_intf, menu, qtr( "Pre&vious" ),<br>
diff --git a/src/libvlc-module.c b/src/libvlc-module.c<br>
index f0ee030..429732e 100644<br>
--- a/src/libvlc-module.c<br>
+++ b/src/libvlc-module.c<br>
</blockquote>
<br></div></div>
Qt and core code should be patched separately.<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
@@ -1136,6 +1136,10 @@ static const char *const ppsz_prefres[] = {<br>
 #define PAS_LONGTEXT N_( \<br>
     "Stop the playlist after each played playlist item." )<br>
<br>
+#define PASO_TEXT N_("Play and stop once")<br>
+#define PASO_LONGTEXT N_( \<br>
+    "Stop the playlist after the next played playlist item." )<br>
+<br>
 #define PAE_TEXT N_("Play and exit")<br>
 #define PAE_LONGTEXT N_( \<br>
     "Exit if there are no more items in the playlist." )<br>
@@ -2628,6 +2632,8 @@ vlc_module_begin ()<br>
     add_string( "bookmark10", NULL,<br>
               BOOKMARK10_TEXT, BOOKMARK_LONGTEXT, false )<br>
<br>
+    add_bool( "play-and-stop-once", 0, PASO_TEXT, PASO_LONGTEXT, false )<br>
</blockquote>
<br></div>
This should not be needed at all, since this is not a persistent parameter.<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
 #define HELP_TEXT \<br>
     N_("print help for VLC (can be combined with --advanced and " \<br>
        "--help-verbose)")<br>
diff --git a/src/playlist/thread.c b/src/playlist/thread.c<br>
index a6ecbdb..dedf3b0 100644<br>
--- a/src/playlist/thread.c<br>
+++ b/src/playlist/thread.c<br>
@@ -359,6 +359,7 @@ static playlist_item_t *NextItem( playlist_t<br>
*p_playlist )<br>
         bool b_loop = var_GetBool( p_playlist, "loop" );<br>
         bool b_repeat = var_GetBool( p_playlist, "repeat" );<br>
         bool b_playstop = var_InheritBool( p_playlist, "play-and-stop" );<br>
+        bool b_playstoponce = var_InheritBool( p_playlist,<br>
"play-and-stop-once" );<br>
<br>
         /* Repeat and play/stop */<br>
         if( b_repeat && get_current_status_item( p_playlist ) )<br>
@@ -371,6 +372,12 @@ static playlist_item_t *NextItem( playlist_t<br>
*p_playlist )<br>
             msg_Dbg( p_playlist,"stopping (play and stop)" );<br>
             return NULL;<br>
         }<br>
+        else if( b_playstoponce )<br>
+        {<br>
+            msg_Dbg( p_playlist, "stopping (play and stop once)" );<br>
+            var_SetBool( p_playlist, "play-and-stop-once", false );<br>
</blockquote>
<br></div></div>
That's a bit racy. I would add an exchange operation to var_GetAndSet().<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            return NULL;<br>
+        }<br>
</blockquote>
<br>
...then you could do:<br>
<br>
+        else if( var_ExchangeBool( p_playlist, "play-and-stop-once", false ) )<div><br>
+        {<br>
+            msg_Dbg( p_playlist, "stopping (play and stop once)" );<br></div><div><div>
+            return NULL;<br>
+        }<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
         /* */<br>
         if( get_current_status_item( p_playlist ) )<br>
</blockquote>
<br>
-- <br></div></div><span><font color="#888888">
Rémi Denis-Courmont<br>
</font></span></blockquote></div></div></div><br></div>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div>