#31 Add libmpdel-get-state to run handler after state is get

Merged
DamienCassou merged 1 commits from vonfry/libmpdel:fix/play-pause into master 4 months ago
vonfry commented 5 months ago

Fix: #30

I add a help function to wait the value changing to non-nil. To avoid blocking main thread, a timer is used. I have no better ideas to do this.

Fix: #30 I add a help function to wait the value changing to non-nil. To avoid blocking main thread, a timer is used. I have no better ideas to do this.
vonfry added 1 commit 5 months ago
vonfry force-pushed fix/play-pause from 8547fca583 to 695f8da40f 5 months ago
DamienCassou approved these changes 5 months ago
DamienCassou left a comment

Thank you very much. I suggested some changes.

libmpdel.el Outdated
(let* ((fstate (intern (format "libmpdel-%s" state)))
timer)
(unless (libmpdel-connected-p)
(libmpdel-refresh-status))
DamienCassou commented 5 months ago

I suggest adding an optional callback to libmpdel-refresh-status. This callback would be called when refreshing is finished.

Then you need to pass the lambda below as argument to libmpdel-refresh-status.

If you do that, the code below don't need a timer anymore as it is only executed when the value is already there (you have to replace the unless above with if).

I suggest adding an optional callback to `libmpdel-refresh-status`. This callback would be called when refreshing is finished. Then you need to pass the lambda below as argument to `libmpdel-refresh-status`. If you do that, the code below don't need a timer anymore as it is only executed when the value is already there (you have to replace the `unless` above with `if`).
vonfry force-pushed fix/play-pause from 695f8da40f to 4565e120c4 5 months ago
vonfry requested review from DamienCassou 5 months ago
DamienCassou approved these changes 5 months ago
DamienCassou left a comment

Here are some more suggestions. Thank you for your great work!

libmpdel.el
failed due to the state is nil."
(let ((fstate (intern (format "libmpdel-%s" state))))
(if (and (funcall fstate) (libmpdel-connected-p))
(funcall handler)
DamienCassou commented 5 months ago

I suggest passing the variable value as argument to handler. Something like (untested):

(let ((call-handler (lambda ()
                      (funcall handler (funcall fstate)))))
  (if (...)
      (funcall call-handler)
    (libmpdel-refresh-status call-handler)))
I suggest passing the variable value as argument to `handler`. Something like (untested): ```emacs (let ((call-handler (lambda () (funcall handler (funcall fstate))))) (if (...) (funcall call-handler) (libmpdel-refresh-status call-handler))) ```
vonfry marked this conversation as resolved
libmpdel.el
(pause "pause 0")
(stop "play"))))
(libmpdel-get-state 'play-state
(lambda ()
DamienCassou commented 5 months ago

if you apply my suggestion above, you get the play-state as argument here so you don't have to call (libmpdel-play-state) below.

if you apply my suggestion above, you get the `play-state` as argument here so you don't have to call `(libmpdel-play-state)` below.
vonfry marked this conversation as resolved
libmpdel.el
If HANDLER is not nil, it will be called after refreshing."
(if handler
(libmpdel-send-command "status"
DamienCassou commented 5 months ago

I suggest only calling libmpdel-send-command once:

(libmpdel-send-command "status"
  (lambda (data)
    (libmpdel--msghandler-status data)
    (when handler (funcall handler))))
  
I suggest only calling `libmpdel-send-command` once: ```emacs (libmpdel-send-command "status" (lambda (data) (libmpdel--msghandler-status data) (when handler (funcall handler)))) ```
vonfry marked this conversation as resolved
vonfry force-pushed fix/play-pause from 4565e120c4 to 8a4adeef79 5 months ago
vonfry force-pushed fix/play-pause from 8a4adeef79 to 27f87b70a1 5 months ago
vonfry force-pushed fix/play-pause from 27f87b70a1 to b0f485e049 5 months ago
vonfry requested review from DamienCassou 5 months ago
vonfry force-pushed fix/play-pause from b0f485e049 to 928857b394 5 months ago
DamienCassou approved these changes 4 months ago
DamienCassou left a comment

Perfect, thank you very much!

DamienCassou merged commit 0ccf76c860 into master 4 months ago

Reviewers

DamienCassou approved these changes 4 months ago
The pull request has been merged as 0ccf76c860.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.