#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 10 months ago
vonfry commented 11 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 11 months ago
vonfry force-pushed fix/play-pause from 8547fca583 to 695f8da40f 11 months ago
DamienCassou approved these changes 11 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 11 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 11 months ago
vonfry requested review from DamienCassou 11 months ago
DamienCassou approved these changes 11 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 11 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 11 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 11 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 11 months ago
vonfry force-pushed fix/play-pause from 8a4adeef79 to 27f87b70a1 11 months ago
vonfry force-pushed fix/play-pause from 27f87b70a1 to b0f485e049 11 months ago
vonfry requested review from DamienCassou 11 months ago
vonfry force-pushed fix/play-pause from b0f485e049 to 928857b394 11 months ago
DamienCassou approved these changes 10 months ago
DamienCassou left a comment

Perfect, thank you very much!

DamienCassou merged commit 0ccf76c860 into master 10 months ago

Reviewers

DamienCassou approved these changes 10 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.