eric/pmomusic

Watch

Fork 0

Sémantiques bizarres autour des stops. #50

New issue

Closed

opened 2026-01-11 07:51:05 +01:00 by eric · 3 comments

eric commented

2026-01-11 07:51:05 +01:00

(Migrated from gargoton.petite-maison-orange.fr)

Dans le renderer, la fonction stop n'arrête pas vraiment la lecture. Sauf si on appelle avant.

renderer.mark_user_stop_requested();

Encore un exemple de commande qui ne fait pas ce qu'elle dit.

Dans le renderer, la fonction stop n'arrête pas vraiment la lecture. Sauf si on appelle avant. ```rust renderer.mark_user_stop_requested(); ``` Encore un exemple de commande qui ne fait pas ce qu'elle dit.

eric added the

question

label

2026-08-29 18:03:44 +02:00

eric commented

2026-08-29 23:29:56 +02:00

Owner

Résumé du problème

MusicRenderer n’est pas le périphérique UPnP/Chromecast/LinkPlay : c’est la représentation locale qu’a le ControlPoint d’un renderer distant. Sa méthode stop() ne fait pas que transmettre une commande de transport — elle mélange deux rôles :

envoyer la commande d’arrêt au backend (le vrai renderer),
gérer de la logique métier interne (auto-advance de la queue).

Or, pour que l’arrêt soit correctement interprété, il faut systématiquement faire :

renderer.mark_user_stop_requested();
renderer.stop()?;

stop() seul ne suffit pas. Le flag user_stop_requested est lu plus tard dans handle_state_change() pour décider si l’état Stopped qui arrive doit déclencher l’auto-advance ou pas. Si le flag n’a pas été positionné avant l’appel, le périphérique s’arrête bien, mais le contrôle point peut relancer le titre suivant de la queue tout seul.

Le ticket a donc raison de trouver ça tordu : la sémantique de stop() ne couvre pas l’intention complète de l’appelant, et le contrat repose sur un appel séparé à une méthode dont le nom (mark_user_stop_requested) ne dit pas qu’il est obligatoire pour un arrêt utilisateur.

Analyse : confusion des responsabilités

Le nœud vient du fait que MusicRenderer héberge un état qui ne devrait pas être le sien :

user_stop_requested est une information métier du Control Point : « quand l’état du renderer passera à Stopped, je dois considérer que c’est un arrêt utilisateur, pas une fin de titre naturelle ».
Laisser ce flag dans MusicRenderer fait de cet objet un hybride entre :
- un adaptateur de transport (qui parle aux backends UPnP, Chromecast, etc.),
- et un acteur métier (qui décide de l’auto-advance).

Du coup, stop() sur MusicRenderer a une signature de commande de transport mais une sémantique qui dépend d’un flag interne. C’est cette ambiguïté qui crée l’API de merde.

Pistes de résolution

1. Extraire la décision « arrêt utilisateur » du MusicRenderer

Supprimer mark_user_stop_requested() et check_and_clear_user_stop_requested() de MusicRenderer.
Laisser MusicRenderer.stop() ne faire que transmettre la commande au backend.
Remonter la logique d’auto-advance au ControlPoint : c’est lui qui sait si l’arrêt vient de l’utilisateur, d’un timer, ou d’une fin de titre naturelle.

Avantage : MusicRenderer redevient un simple reflet de l’état observé du périphérique, sans logique métier cachée.

2. Si on garde la logique dans MusicRenderer, rendre le contrat explicite

Introduire une méthode stop_user() ou stop_with_intent(StopIntent::User) qui encapsule les deux appels.
stop() reste une commande de transport brute ; la nouvelle méthode est celle à utiliser quand c’est l’utilisateur qui arrête la lecture.

Avantage : pas de changement d’architecture, mais le contrat est visible dans la signature.

3. Rétro-compatibilité

Vérifier tous les appelleurs : control_point.rs (lignes 393 et 1092), les backends, les exemples (full_control_point_demo, pmo_remote_control), les tests.
Si on supprime le flag de MusicRenderer, il faut reproduire le comportement dans ControlPoint (stocker l’intention d’arrêt, la consommer dans le handler d’état).

Conclusion

Ce n’est pas un bug fonctionnel : stop() arrête bien la lecture. C’est un problème de conception de l’API : la séparation entre « commande de transport » et « intention métier » n’est pas respectée, et elle est cachée dans un objet (MusicRenderer) qui ne devrait pas porter cette décision. Je propose de garder le ticket ouvert pour étudier la piste #1 (remonter la logique au ControlPoint) quand on aura le temps.

**Résumé du problème** `MusicRenderer` n’est pas le périphérique UPnP/Chromecast/LinkPlay : c’est la **représentation locale** qu’a le `ControlPoint` d’un renderer distant. Sa méthode `stop()` ne fait pas que transmettre une commande de transport — elle mélange deux rôles : 1. envoyer la commande d’arrêt au backend (le vrai renderer), 2. gérer de la logique métier interne (auto-advance de la queue). Or, pour que l’arrêt soit correctement interprété, il faut systématiquement faire : ```rust renderer.mark_user_stop_requested(); renderer.stop()?; ``` `stop()` seul ne suffit pas. Le flag `user_stop_requested` est lu plus tard dans `handle_state_change()` pour décider si l’état `Stopped` qui arrive doit déclencher l’auto-advance ou pas. Si le flag n’a pas été positionné avant l’appel, le périphérique s’arrête bien, mais le contrôle point peut relancer le titre suivant de la queue tout seul. Le ticket a donc raison de trouver ça tordu : la sémantique de `stop()` ne couvre pas l’intention complète de l’appelant, et le contrat repose sur un appel séparé à une méthode dont le nom (`mark_user_stop_requested`) ne dit pas qu’il est obligatoire pour un arrêt utilisateur. --- **Analyse : confusion des responsabilités** Le nœud vient du fait que `MusicRenderer` héberge un état qui ne devrait pas être le sien : - `user_stop_requested` est une information **métier du Control Point** : « quand l’état du renderer passera à Stopped, je dois considérer que c’est un arrêt utilisateur, pas une fin de titre naturelle ». - Laisser ce flag dans `MusicRenderer` fait de cet objet un hybride entre : - un adaptateur de transport (qui parle aux backends UPnP, Chromecast, etc.), - et un acteur métier (qui décide de l’auto-advance). Du coup, `stop()` sur `MusicRenderer` a une signature de commande de transport mais une sémantique qui dépend d’un flag interne. C’est cette ambiguïté qui crée l’API de merde. --- **Pistes de résolution** **1. Extraire la décision « arrêt utilisateur » du `MusicRenderer`** - Supprimer `mark_user_stop_requested()` et `check_and_clear_user_stop_requested()` de `MusicRenderer`. - Laisser `MusicRenderer.stop()` ne faire que transmettre la commande au backend. - Remonter la logique d’auto-advance au `ControlPoint` : c’est lui qui sait si l’arrêt vient de l’utilisateur, d’un timer, ou d’une fin de titre naturelle. Avantage : `MusicRenderer` redevient un simple reflet de l’état observé du périphérique, sans logique métier cachée. **2. Si on garde la logique dans `MusicRenderer`, rendre le contrat explicite** - Introduire une méthode `stop_user()` ou `stop_with_intent(StopIntent::User)` qui encapsule les deux appels. - `stop()` reste une commande de transport brute ; la nouvelle méthode est celle à utiliser quand c’est l’utilisateur qui arrête la lecture. Avantage : pas de changement d’architecture, mais le contrat est visible dans la signature. **3. Rétro-compatibilité** - Vérifier tous les appelleurs : `control_point.rs` (lignes 393 et 1092), les backends, les exemples (`full_control_point_demo`, `pmo_remote_control`), les tests. - Si on supprime le flag de `MusicRenderer`, il faut reproduire le comportement dans `ControlPoint` (stocker l’intention d’arrêt, la consommer dans le handler d’état). --- **Conclusion** Ce n’est pas un bug fonctionnel : `stop()` arrête bien la lecture. C’est un problème de conception de l’API : la séparation entre « commande de transport » et « intention métier » n’est pas respectée, et elle est cachée dans un objet (`MusicRenderer`) qui ne devrait pas porter cette décision. Je propose de garder le ticket ouvert pour étudier la piste #1 (remonter la logique au `ControlPoint`) quand on aura le temps.

eric commented

2026-08-29 23:33:53 +02:00

Owner

Je vois des appels dans control_point.rs (lignes 396, 1096, 1362) et dans les exemples. Je vais vérifier les contextes complets, notamment si tous les appelleurs passent bien par mark_user_stop_requested() avant.

Il y a des appelleurs qui ne passent pas par mark_user_stop_requested(). Je vais vérifier les exemples et le transfert de playlist.

Je vois des appelleurs qui ne positionnent pas le flag. Je vais vérifier le dernier exemple.

Voici les appelleurs de MusicRenderer::stop() :

Qui positionne bien le flag avant :

control_point.rs ligne 393-396 : timer de sommeil expiré
control_point.rs ligne 1092-1096 : user_stop() (bouton Stop UI)

Qui n’y passent pas :

control_point.rs ligne 1362 : transfert de playlist d’un renderer source vers un renderer destination — arrêt “interne”, pas un arrêt utilisateur
musicrenderer.rs ligne 2005 : shuffle_queue() — arrêt interne pour relancer la lecture mélangée
pmocontrol/examples/full_control_point_demo.rs ligne 798 : stop_renderer() — démo, arrêt “utilisateur” sans le flag
pmocontrol/examples/full_control_point_demo.rs ligne 870 : stop_current_renderer_playback() — même problème
pmocontrol/examples/renderer_demo.rs ligne 205 : démo du renderer, stop “brut”

Ce que ça montre :

L’API oblige chaque appelleur à connaître le contrat implicite “il faut appeler mark_user_stop_requested() avant stop() pour un arrêt utilisateur”.
Les exemples eux-mêmes ne respectent pas ce contrat, donc ils sont bugués vis-à-vis de l’auto-advance.
Les arrêts “internes” (transfert, shuffle) ne devraient d’ailleurs pas positionner le flag, mais rien dans l’API ne distingue les deux cas.

C’est exactement le nœud : la responsabilité de “savoir si c’est un arrêt utilisateur” est dispersée chez tous les appelleurs, alors qu’elle devrait être encapsulée dans l’API.

Je vois des appels dans `control_point.rs` (lignes 396, 1096, 1362) et dans les exemples. Je vais vérifier les contextes complets, notamment si tous les appelleurs passent bien par `mark_user_stop_requested()` avant. Il y a des appelleurs qui ne passent pas par `mark_user_stop_requested()`. Je vais vérifier les exemples et le transfert de playlist. Je vois des appelleurs qui ne positionnent pas le flag. Je vais vérifier le dernier exemple. Voici les appelleurs de `MusicRenderer::stop()` : **Qui positionne bien le flag avant :** - `control_point.rs` ligne 393-396 : timer de sommeil expiré - `control_point.rs` ligne 1092-1096 : `user_stop()` (bouton Stop UI) **Qui n’y passent pas :** - `control_point.rs` ligne 1362 : transfert de playlist d’un renderer source vers un renderer destination — arrêt “interne”, pas un arrêt utilisateur - `musicrenderer.rs` ligne 2005 : `shuffle_queue()` — arrêt interne pour relancer la lecture mélangée - `pmocontrol/examples/full_control_point_demo.rs` ligne 798 : `stop_renderer()` — démo, arrêt “utilisateur” sans le flag - `pmocontrol/examples/full_control_point_demo.rs` ligne 870 : `stop_current_renderer_playback()` — même problème - `pmocontrol/examples/renderer_demo.rs` ligne 205 : démo du renderer, stop “brut” **Ce que ça montre :** - L’API oblige chaque appelleur à connaître le contrat implicite “il faut appeler `mark_user_stop_requested()` avant `stop()` pour un arrêt utilisateur”. - Les exemples eux-mêmes ne respectent pas ce contrat, donc ils sont bugués vis-à-vis de l’auto-advance. - Les arrêts “internes” (transfert, shuffle) ne devraient d’ailleurs pas positionner le flag, mais rien dans l’API ne distingue les deux cas. C’est exactement le nœud : la responsabilité de “savoir si c’est un arrêt utilisateur” est dispersée chez tous les appelleurs, alors qu’elle devrait être encapsulée dans l’API.

eric commented

2026-08-30 09:47:58 +02:00

Owner

C’est bon, c’est démêlé et corrigé.

eric closed this issue

2026-08-30 09:47:58 +02:00