org-player and switch to lexical binding in org.el

org-player and switch to lexical binding in org.el

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

Hi all

release_8.3.3-426-g1f49e9f introduces a regression. The link

    [[file:/dir/audio.mp3::0:12:34]]

results in

    Debugger entered--Lisp error: (void-variable search)
      (org-player-play-file file search)
      [...]
      org-open-file("/dir/audio.mp3" nil nil "0:12:34")
      [...]

I hope it is reproducible after (require 'org-player), org-player.el
attached.

Michael

[-- Attachment #2: org-player.el --]
[-- Type: application/octet-stream, Size: 10372 bytes --]

;;; -*- coding: utf-8-unix -*-
;;; org-player.el - Play audio and video files in org-mode hyperlinks
;;;
;;; Author: Paul Sexton <eeeickythump@gmail.com>
;;; Version: 1.0.0
;;; Repository at http://bitbucket.org/eeeickythump/org-player/
;;;
;;;
;;; Synopsis
;;; ========
;;;
;;; I was learning a language, and wanted to include hyperlinks to audio files
;;; within my org document, and to be able to play each file by clicking on its
;;; link. I wrote the code in this file, which makes org mode use an Emacs
;;; media player library, Bongo, to play audio and video files contained in
;;; 'file:' hyperlinks.
;;;
;;; Installation
;;; ============
;;;
;;; First you will need to install Bongo, which you can download from:
;;; https://github.com/dbrock/bongo
;;;
;;; You will also need to ensure you have an actual media player program
;;; installed, and you need to tell Bongo which program to use to play files. I
;;; have tested org-player with MPlayer, an open source media player which
;;; works well on Windows and Linux. VLC does not work with Bongo on Windows at
;;; the time of writing.
;;;
;;; You can obtain MPlayer from:
;;; http://www.mplayerhq.hu/design7/news.html
;;;
;;; Ensure you have Bongo set up and working. Once this is achieved, installing
;;; org-player is easy - just put the file somewhere in your Emacs load-path
;;; and put (require 'org-player) in your .emacs file.
;;;
;;; Usage
;;; =====
;;;
;;; Clicking on a link such as
;;;
;;;   [[file:/path/to/song.mp3]]
;;;
;;; adds it to the active Bongo playlist and immediately starts playing
;;; it. Playback can be paused, fast-forwarded etc using Bongo commands (see
;;; below).
;;;
;;; Links can also specify track positions. When a link contains a track
;;; position, playback will start at that position in the track. For example:
;;;
;;; [[file:/path/to/song.mp3::2:43]]      Starts playback at 2 min 43 sec.
;;; [[file:/path/to/song.mp3::1:10:45]]   Starts playback at 1 hr 10 min 45 sec.
;;; [[file:/path/to/song.mp3::3m15s]]     Starts playback at 3 min 15 sec.
;;; [[file:/path/to/song.mp3::49s]]       Starts playback at 0 min 49 sec.
;;; [[file:/path/to/song.mp3::1h21m10s]]  Starts playback at 1 hr 21 min 10 sec.
;;;
;;; Controlling playback
;;; ====================
;;;
;;; When Bongo plays a file it puts some icons in the modeline that resemble the
;;; well-known symbols for 'play', 'stop', 'rewind' and so on, and which can be
;;; used to control playback using the mouse. I found these worked erratically when
;;; outside the actual Bongo playlist buffer, so I have instead bound some org-mode
;;; keys (ctrl + numpad keys) to the relevant functions.  These are:
;;;
;;; C-<keypad 0>     pause/resume
;;; C-<keypad .>     stop, or restart playback from beginning
;;; C-<keypad />     show track info
;;; C-<keypad 4>     skip back 10 seconds
;;; C-<keypad 6>     skip forward 10 seconds
;;; C-<keypad 9>     increase volume (requires separate 'volume' library
;;;                  at https://github.com/dbrock/volume-el )
;;; C-<keypad 3>     decrease volume
;;;
;;; Note that these keys (and the modeline icons, if they work) act on the
;;; track that is active in the Bongo playlist. This will always be the last
;;; track that you added by clicking on a link in org-mode, unless you alter
;;; the Bongo playlist outside org mode.
;;;
;;; Also note that these keys are only bound within org mode buffers.
;;;
;;; Notes
;;; =====
;;;
;;; I have only tested this with MP3 files, but it ought to work with
;;; video as well, as both MPlayer and Bongo claim to work with video files.
;;;
;;; Future plans
;;; ============
;;;
;;; If anyone wants to integrate this code with EMMS, another popular Emacs
;;; media player library, contributions would be welcome.


(require 'org)
(require 'bongo)

(defvar org-player-file-extensions-regexp
  (concat "\\." (regexp-opt
                 (append bongo-audio-file-name-extensions
                         bongo-video-file-name-extensions))))

(add-to-list 'org-file-apps
             (cons (concat org-player-file-extensions-regexp "$")
                   '(org-player-play-file file)))

(add-to-list 'org-file-apps
             (cons (concat org-player-file-extensions-regexp
                           "::[0-9]+:[0-9]+\\(:[0-9]+\\|\\)")
                   '(org-player-play-file file search)))

(add-to-list 'org-file-apps
             (cons (concat org-player-file-extensions-regexp
                           "::[0-9]+[HhMmSs]\\([0-9]+[MmSs]\\|\\)\\([0-9]+[MmSs]\\|\\)")
                   '(org-player-play-file file search)))


(defun org-player-string-to-time (str)
  "Understands any of the following formats: MM:SS, HH:MM:SS, NNhNNmNNs,
NNhNNm, NNmNNs, NNh, NNm, NNs."
  (let ((str (substring-no-properties str))
        hours mins secs)
    (cond
     ((string-match "\\([0-9]+\\):\\([0-9]+\\)\\(:[0-9.]+\\|\\)" str)
      (if (string= "" (match-string 3 str)) ; XX:YY
          (setq mins (match-string 1 str)
                secs (match-string 2 str))
        (setq hours (match-string 1 str) ; XX:YY:ZZ
              mins (match-string 2 str)
              secs (subseq (match-string 3 str) 1))))
     ((eq (string-match "[0-9]+[HhMmSs]" str)
          (string-match "\\([0-9]+[Hh]\\|\\)\\([0-9]+[Mm]\\|\\)\\([0-9.]+[Ss]\\|\\)"
                        str))
      (setq hours (match-string 1 str)
            mins (match-string 2 str)
            secs (match-string 3 str))
      (unless (string= "" hours)
        (setq hours (subseq hours 0 (1- (length hours)))))
      (unless (string= "" mins)
        (setq mins (subseq mins 0 (1- (length mins)))))
      (unless (string= "" secs)
        (setq secs (subseq secs 0 (1- (length secs))))))
     (t
      (error "Unrecognised track position specifier: %S" str)))
    (setq hours (if (or (null hours) (string= "" hours))
                    0 (read hours)))
    (setq mins (if (or (null mins) (string= "" mins))
                    0 (read mins)))
    (setq secs (if (or (null secs) (string= "" secs))
                    0 (read secs)))
    (list hours mins secs)))


(defun org-player-play-file (filename &optional pos)
  "POS, if given, is a string that specifies a track position where
playback should begin. This string is taken from the portion of the
hyperlink that follows a double colon. For example:

  [[file:song.mp3::03:22]]
  [[file:song.mp3::1h24m3s]]

See `org-player-string-to-time' for a description of the format of
the POS string."
  (let ((seek (if pos
                  (destructuring-bind (hours mins secs)
                      (org-player-string-to-time pos)
                    (+ secs
                       (* mins 60)
                       (* hours 60 60))))))
    (with-bongo-buffer
      (bongo-insert-file filename)
      (backward-char)
      (org-player-start/stop 'play seek)
      (when seek
        (bongo-seek-to seek)))))


(defun org-player-get-track-info ()
  "Returns a list: (TRACKTITLE ALBUMNAME ARTISTNAME ELAPSEDSECS LENGTHSECS)"
  (with-bongo-buffer
    (let* ((info (bongo-line-infoset))
           (track (cdr (assoc 'track info))))
      (message "%s" info)
      (list (cdr (assoc 'title track))
            (cdr (assoc 'album info))
            (cdr (assoc 'artist info))
            (or (bongo-elapsed-time) 0)
            (or (bongo-total-time) 0)))))


(defun org-player-print-track-info ()
  "Print out some info about the active track, in the minibuffer."
  (interactive)
  (destructuring-bind (title album artist elapsed total)
      (org-player-get-track-info)
    (message "%s" (list title album artist elapsed total))
    (cond
     ((null title)
      (message "No active track."))
     (t
      (message "Track: %s
Album: %s / %s
%s / %s (%d%%) %s"
               title album artist
               (format "%02d:%02d" (floor elapsed 60) (mod elapsed 60))
               (format "%02d:%02d" (floor total 60) (mod total 60))
               (/ (* elapsed 100) total)
               (cond
                ((bongo-paused-p) "(paused)")
                ((bongo-playing-p) "(playing)")
                (t "(stopped)"))
               )))))


(defun org-player-pause/resume ()
  (interactive)
  (destructuring-bind (title album artist elapsed total)
      (org-player-get-track-info)
    (when title
      (message "%02d:%02d/%02d:%02d %s %s (%s / %s)"
               (floor elapsed 60) (mod elapsed 60)
               (floor total 60) (mod total 60)
               (if (bongo-paused-p) "Unpaused:" "Paused:")
               title album artist)))
    (bongo-pause/resume))


(defun org-player-start/stop (&optional force starting-pos)
  (interactive)
  (destructuring-bind (title album artist elapsed total)
      (org-player-get-track-info)
    (when title
      (cond
       ((eq force 'play)
        (bongo-play))
       ((eq force 'stop)
        (bongo-stop))
       (t
        (bongo-start/stop)))
      (let ((msg (format "%02d:%02d/%02d:%02d %s %s (%s / %s)"
                         (floor (or starting-pos 0) 60)
                         (mod (or starting-pos 0) 60)
                         (floor total 60) (mod total 60)
                         (if (bongo-playing-p) "Playing:" "Stopped:")
                         title album artist)))
        (message msg)))))


;; Numpad Ctrl-0: pause/resume
(define-key org-mode-map (kbd "<C-kp-0>") 'org-player-pause/resume)
(define-key org-mode-map (kbd "<C-kp-insert>") 'org-player-pause/resume)
;; Numpad Ctrl-.: stop current track, or restart from beginning if stopped
(define-key org-mode-map (kbd "<C-kp-decimal>") 'org-player-start/stop)
(define-key org-mode-map (kbd "<C-kp-delete>") 'org-player-start/stop)
;; Numpad Ctrl-PgUp, Ctrl-PgDn: raise/lower volume
(define-key org-mode-map (kbd "<C-kp-prior>") 'volume-raise)
(define-key org-mode-map (kbd "<C-kp-next>") 'volume-lower)
(define-key org-mode-map (kbd "<C-kp-9>") 'volume-raise)
(define-key org-mode-map (kbd "<C-kp-3>") 'volume-lower)
;; Numpad Ctrl-left, Ctrl-right: skip back/forward 10 seconds
(define-key org-mode-map (kbd "<C-kp-left>") 'bongo-seek-backward-10)
(define-key org-mode-map (kbd "<C-kp-right>") 'bongo-seek-forward-10)
(define-key org-mode-map (kbd "<C-kp-4>") 'bongo-seek-backward-10)
(define-key org-mode-map (kbd "<C-kp-6>") 'bongo-seek-forward-10)
;; Ctrl-/: show track info
(define-key org-mode-map (kbd "<C-kp-divide>") 'org-player-print-track-info)


(provide 'org-player)

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> release_8.3.3-426-g1f49e9f introduces a regression. The link
>
>     [[file:/dir/audio.mp3::0:12:34]]
>
> results in
>
>     Debugger entered--Lisp error: (void-variable search)
>       (org-player-play-file file search)
>       [...]
>       org-open-file("/dir/audio.mp3" nil nil "0:12:34")
>       [...]

`search' never was advertised as a dynamically scoped variable in
`org-file-apps' docstring, so "org-player" is just playing with fire
here.

I don't like the current solution either (eval with a LEXICAL argument).

I think it would be better to use un function with two arguments (file
and link-string instead). This is not backward compatible, but the
change is trivial: sexp -> (lambda (file link) sexp).

In the current case, you need to use match string:

  (add-to-list 'org-file-apps
               (cons (concat org-player-file-extensions-regexp
                             "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
                     (lambda (file link)
                       (org-player-play-file file (match-string 1 link)))))

WDYT?


Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

Hi Paul

It seems that this is beyond of my knowledge and I would like to ask
you as the author of org-player for help.

On Sun, Jan 17, 2016 at 9:40 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> Hello,
>
> Michael Brand <michael.ch.brand@gmail.com> writes:
>
>> release_8.3.3-426-g1f49e9f introduces a regression. The link
>>
>>     [[file:/dir/audio.mp3::0:12:34]]
>>
>> results in
>>
>>     Debugger entered--Lisp error: (void-variable search)
>>       (org-player-play-file file search)
>>       [...]
>>       org-open-file("/dir/audio.mp3" nil nil "0:12:34")
>>       [...]
>
> `search' never was advertised as a dynamically scoped variable in
> `org-file-apps' docstring, so "org-player" is just playing with fire
> here.
>
> I don't like the current solution either (eval with a LEXICAL argument).
>
> I think it would be better to use un function with two arguments (file
> and link-string instead). This is not backward compatible, but the
> change is trivial: sexp -> (lambda (file link) sexp).
>
> In the current case, you need to use match string:
>
>   (add-to-list 'org-file-apps
>                (cons (concat org-player-file-extensions-regexp
>                              "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>                      (lambda (file link)
>                        (org-player-play-file file (match-string 1 link)))))
>
> WDYT?
>
>
> Regards,
>
> --
> Nicolas Goaziou

Michael

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

Hi Paul

It seems that this is beyond of my knowledge and I would like to ask
you as the author of org-player for help.


On Sun, Jan 17, 2016 at 9:40 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> Hello,
>
> Michael Brand <michael.ch.brand@gmail.com> writes:
>
>> release_8.3.3-426-g1f49e9f introduces a regression. The link
>>
>>     [[file:/dir/audio.mp3::0:12:34]]
>>
>> results in
>>
>>     Debugger entered--Lisp error: (void-variable search)
>>       (org-player-play-file file search)
>>       [...]
>>       org-open-file("/dir/audio.mp3" nil nil "0:12:34")
>>       [...]
>
> `search' never was advertised as a dynamically scoped variable in
> `org-file-apps' docstring, so "org-player" is just playing with fire
> here.
>
> I don't like the current solution either (eval with a LEXICAL argument).
>
> I think it would be better to use un function with two arguments (file
> and link-string instead). This is not backward compatible, but the
> change is trivial: sexp -> (lambda (file link) sexp).
>
> In the current case, you need to use match string:
>
>   (add-to-list 'org-file-apps
>                (cons (concat org-player-file-extensions-regexp
>                              "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>                      (lambda (file link)
>                        (org-player-play-file file (match-string 1 link)))))
>
> WDYT?
>
>
> Regards,
>
> --
> Nicolas Goaziou

Michael

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

Hi Nicolas

On Sun, Jan 17, 2016 at 9:40 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> Hello,
>
> Michael Brand <michael.ch.brand@gmail.com> writes:
>
>> release_8.3.3-426-g1f49e9f introduces a regression. The link
>>
>>     [[file:/dir/audio.mp3::0:12:34]]
>>
>> results in
>>
>>     Debugger entered--Lisp error: (void-variable search)
>>       (org-player-play-file file search)
>>       [...]
>>       org-open-file("/dir/audio.mp3" nil nil "0:12:34")
>>       [...]
>
> `search' never was advertised as a dynamically scoped variable in
> `org-file-apps' docstring, so "org-player" is just playing with fire
> here.
>
> I don't like the current solution either (eval with a LEXICAL argument).
>
> I think it would be better to use un function with two arguments (file
> and link-string instead). This is not backward compatible, but the
> change is trivial: sexp -> (lambda (file link) sexp).
>
> In the current case, you need to use match string:
>
>   (add-to-list 'org-file-apps
>                (cons (concat org-player-file-extensions-regexp
>                              "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>                      (lambda (file link)
>                        (org-player-play-file file (match-string 1 link)))))
>
> WDYT?

I am still not able to get your suggestion that Paul has implemented
in the meantime in org-player.el 1.0.1 to work. Now I am trying to
understand what I am missing with the lambda.

Has the (eval cmd) in org-open-file to be extended somehow to the
variant (funcall cmd) as lambda evaluates to itself?

Michael

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> On Sun, Jan 17, 2016 at 9:40 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>
>> In the current case, you need to use match string:
>>
>>   (add-to-list 'org-file-apps
>>                (cons (concat org-player-file-extensions-regexp
>>                              "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>>                      (lambda (file link)
>>                        (org-player-play-file file (match-string 1 link)))))
>>
>> WDYT?
>
> I am still not able to get your suggestion that Paul has implemented
> in the meantime in org-player.el 1.0.1 to work. Now I am trying to
> understand what I am missing with the lambda.
>
> Has the (eval cmd) in org-open-file to be extended somehow to the
> variant (funcall cmd) as lambda evaluates to itself?

My suggestion was hypothetical, and not yet implemented. No wonder it
doesn't work.

If you think this change sounds reasonable, I can implement it, tho.
However, there is some backward incompatibility involved.

The current solution, i.e., using `eval' only provides `file' symbol.


Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

Hi Nicolas

On Fri, Jan 29, 2016 at 2:32 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> My suggestion was hypothetical, and not yet implemented. No wonder it
> doesn't work.
>
> If you think this change sounds reasonable, I can implement it, tho.
> However, there is some backward incompatibility involved.
>
> The current solution, i.e., using `eval' only provides `file' symbol.

Only slowly I begin to get it partially. My observation is that if the
current `org-open-file' would be changed to

    (eval cmd
          ;; LEXICAL argument.
          `((file . ,(convert-standard-filename file))
            (link . ,dlink)))

to provide also the `link' symbol then a

    (add-to-list 'org-file-apps
                 (cons (concat org-player-file-extensions-regexp
                               "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
                       '(org-player-play-file file (match-string 1 link))))

which is simple enough for me to understand in org-player.el works.
This situation looks favorable to me at least for a first step because
it would mean a version of org-player.el that remains compatible with
"any" Org before lexical binding in org.el
(release_8.3.3-426-g1f49e9f) but would also become compatible again
starting with one of the next commits in Org master.

Do I understand correct that this would not break any backward
compatibility with all other existing and correct use of
`org-file-apps'?

On Sun, Jan 17, 2016 at 9:40 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> I don't like the current solution either (eval with a LEXICAL argument).
>
> I think it would be better to use un function with two arguments (file
> and link-string instead). This is not backward compatible, but the
> change is trivial: sexp -> (lambda (file link) sexp).

I am concerned about backward compatibility and don't understand the
lambda part enough: Would backward compatibility be broken only for
org-player.el or also for other existing and correct use of
`org-file-apps'?

Or can `org-open-file' support both for some time like this?:

    (if (eq (car cmd) 'lambda)
        ;; Function.
        (funcall cmd (convert-standard-filename file) dlink)
      ;; Sexp, deprecated.
      (eval cmd `((file . ,(convert-standard-filename file))
                  (link . ,dlink))))

Michael

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> Only slowly I begin to get it partially. My observation is that if the
> current `org-open-file' would be changed to
>
>     (eval cmd
>           ;; LEXICAL argument.
>           `((file . ,(convert-standard-filename file))
>             (link . ,dlink)))

Actually, that should be (eval cmd t). Providing an alist is not
necessary here.

> to provide also the `link' symbol then a
>
>     (add-to-list 'org-file-apps
>                  (cons (concat org-player-file-extensions-regexp
>                                "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>                        '(org-player-play-file file (match-string 1 link))))
>
> which is simple enough for me to understand in org-player.el works.
> This situation looks favorable to me at least for a first step because
> it would mean a version of org-player.el that remains compatible with
> "any" Org before lexical binding in org.el
> (release_8.3.3-426-g1f49e9f) but would also become compatible again
> starting with one of the next commits in Org master.
>
> Do I understand correct that this would not break any backward
> compatibility with all other existing and correct use of
> `org-file-apps'?

Although it does the job, it would leave an `eval' in the code base,
which is not very pretty, and more difficult to maintain (scope is less
obvious). That's why I prefer the functions. Of course, it may not be
worth the trouble if introduced backward incompatibility is really
nasty.


Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

Hi Nicolas and Paul

On Sun, Jan 31, 2016 at 12:30 AM, Nicolas Goaziou
<mail@nicolasgoaziou.fr> wrote:

> Michael Brand <michael.ch.brand@gmail.com> writes:
>
>> Only slowly I begin to get it partially. My observation is that if the
>> current `org-open-file' would be changed to
>>
>>     (eval cmd
>>           ;; LEXICAL argument.
>>           `((file . ,(convert-standard-filename file))
>>             (link . ,dlink)))
>
> Actually, that should be (eval cmd t). Providing an alist is not
> necessary here.
>
>> to provide also the `link' symbol then a
>>
>>     (add-to-list 'org-file-apps
>>                  (cons (concat org-player-file-extensions-regexp
>>                                "::\\([0-9]+:[0-9]+\\(:[0-9]+\\)?\\)")
>>                        '(org-player-play-file file (match-string 1 link))))
>>
>> which is simple enough for me to understand in org-player.el works.
>> This situation looks favorable to me at least for a first step because
>> it would mean a version of org-player.el that remains compatible with
>> "any" Org before lexical binding in org.el
>> (release_8.3.3-426-g1f49e9f) but would also become compatible again
>> starting with one of the next commits in Org master.
>>
>> Do I understand correct that this would not break any backward
>> compatibility with all other existing and correct use of
>> `org-file-apps'?
>
> Although it does the job, it would leave an `eval' in the code base,
> which is not very pretty, and more difficult to maintain (scope is less
> obvious). That's why I prefer the functions. Of course, it may not be
> worth the trouble if introduced backward incompatibility is really
> nasty.

I see the pros and cons of either solution with a slight bias to not
break backward compatibility and am OK with what you and others
decide.

(Corrigenda: In my code examples earlier in this thread I should have
used `link' instead of `dlink' which is ~(downcase link)~.)

Michael

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> I see the pros and cons of either solution with a slight bias to not
> break backward compatibility and am OK with what you and others
> decide.

I replaced S-expressions with functions of two arguments. The Org world
is a better place with one less `eval'.

Thank you for the feedback.

Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Hi Nicolas and Paul

On Wed, Feb 3, 2016 at 6:33 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:

> I replaced S-expressions with functions of two arguments. The Org world
> is a better place with one less `eval'.

Just to confirm: Updating org-player.el to 1.0.1 makes Org links to
media files play again. Thank you both for your changes.

I made some thoughts about the user experience of this incompatible
change:

Following such an Org link after updating Org without taking too much
care of `org-file-apps' or as a new Org user silently falls back to
open the file e. g. directly in an Emacs buffer just as with a default
`org-file-apps'. In this situation a user may well not be aware of
that `org-file-apps' has fallen short of an application registered
there. Or, even if I read the Org News I might not be aware of all the
additions to `org-file-apps' I got under the hood. Or the user does
not read the Org News or does not understand the consequences to its
very end.

For all this I suggest a simple migration hint like the error in my
attached patch. Did I miss its disadvantage?

Michael

[-- Attachment #2: 0001-org-file-apps-add-migration-hint.patch --]
[-- Type: application/octet-stream, Size: 859 bytes --]

From b3c3cd9f07e2009068bf5952963d98924b5e4d17 Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.brand@alumni.ethz.ch>
Date: Wed, 3 Feb 2016 21:37:09 +0100
Subject: [PATCH] `org-file-apps' add migration hint

* lisp/org.el (org-file-apps): Add an error when still a sexp is in
use.
---
 lisp/org.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index c93e4e1..c2f945f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11326,6 +11326,8 @@ If the file does not exist, an error is thrown."
       (save-match-data
 	(set-match-data link-match-data)
 	(funcall cmd file link)))
+     ((consp cmd)
+      (error "Please see Org News for Version 9.0 about `org-file-apps'"))
      (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
     (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.4.9 (Apple Git-60)

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> For all this I suggest a simple migration hint like the error in my
> attached patch. Did I miss its disadvantage?

I have no objection, but I suggest to add a big fat FIXME above so as to
remove it once we release Org 9.1.

Also, it should be `user-error', not `error'.

Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

Hi Nicolas

On Wed, Feb 3, 2016 at 9:56 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:

> I have no objection, but I suggest to add a big fat FIXME above so as to
> remove it once we release Org 9.1.

I would prefer to keep it much longer for those who do not regularly
update Org. The many cases when Org 9.0 is not part of the update
path. E. g. when a distro will include Emacs 25.1 with Org 8.3.3 (?)
in some time and a user will start with Org of this distro when it has
already some age and again some time later he will update to Org from
master and a compatible Emacs. I guess that more than a few distros
will skip Org 9.0 and more than a few distro users will also skip some
update steps of their distro.

> Also, it should be `user-error', not `error'.

Intermediate patch version attached.

Michael

[-- Attachment #2: 0001-org-file-apps-add-migration-hint.patch --]
[-- Type: application/octet-stream, Size: 1051 bytes --]

From 5242d8d0008468f6581b40ae7dda3d5e535c312c Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.brand@alumni.ethz.ch>
Date: Wed, 3 Feb 2016 22:46:43 +0100
Subject: [PATCH] `org-file-apps' add migration hint

* lisp/org.el (org-file-apps): Add an error when still a sexp is in
use.
---
 lisp/org.el | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index c93e4e1..b629b2d 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11326,6 +11326,12 @@ If the file does not exist, an error is thrown."
       (save-match-data
 	(set-match-data link-match-data)
 	(funcall cmd file link)))
+     ((consp cmd)
+      ;; Heads-up instead of silently fall back to
+      ;; `org-link-frame-setup' for an old usage of `org-file-apps'
+      ;; with sexp instead of a function for `cmd'.
+      (user-error
+       "Please see Org News for Version 9.0 about `org-file-apps'"))
      (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
     (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.4.9 (Apple Git-60)

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> I would prefer to keep it much longer for those who do not regularly
> update Org. The many cases when Org 9.0 is not part of the update
> path. E. g. when a distro will include Emacs 25.1 with Org 8.3.3 (?)
> in some time and a user will start with Org of this distro when it has
> already some age and again some time later he will update to Org from
> master and a compatible Emacs. I guess that more than a few distros
> will skip Org 9.0 and more than a few distro users will also skip some
> update steps of their distro.

I do not mind, as there is an explicit "FIXME" or "XXX" somewhere above
reminding us to remove this at some point.


Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

Hi Nicolas

On Thu, Feb 4, 2016 at 9:36 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:

> I do not mind, as there is an explicit "FIXME" or "XXX" somewhere above
> reminding us to remove this at some point.

My concern is only the version 9.1 which I find too early. A FIXME in
my sense is added to the attached patch, please rephrase if necessary.

Michael

[-- Attachment #2: 0001-org-file-apps-add-migration-hint.patch --]
[-- Type: application/octet-stream, Size: 1162 bytes --]

From 3647b622b57244e470df045ea31f1a6c6fcb0c82 Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.brand@alumni.ethz.ch>
Date: Thu, 4 Feb 2016 12:42:08 +0100
Subject: [PATCH] `org-file-apps' add migration hint

* lisp/org.el (org-file-apps): Add an error when still a sexp is in
use.
---
 lisp/org.el | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lisp/org.el b/lisp/org.el
index c93e4e1..9c6f5d0 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11326,6 +11326,14 @@ If the file does not exist, an error is thrown."
       (save-match-data
 	(set-match-data link-match-data)
 	(funcall cmd file link)))
+     ((consp cmd)
+      ;; FIXME: Remove this check when most default installations of
+      ;; Emacs have at least Org 9.0.
+      ;; Heads-up instead of silently fall back to
+      ;; `org-link-frame-setup' for an old usage of `org-file-apps'
+      ;; with sexp instead of a function for `cmd'.
+      (user-error
+       "Please see Org News for version 9.0 about `org-file-apps'"))
      (t (funcall (cdr (assq 'file org-link-frame-setup)) file)))
     (and (derived-mode-p 'org-mode)
 	 (eq old-mode 'org-mode)
-- 
2.4.9 (Apple Git-60)

Re: org-player and switch to lexical binding in org.el

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> My concern is only the version 9.1 which I find too early. A FIXME in
> my sense is added to the attached patch, please rephrase if necessary.

It looks good. Thank you.

Regards,

-- 
Nicolas Goaziou

Re: org-player and switch to lexical binding in org.el

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

Hi Paul

On Thu, Jan 28, 2016 at 4:00 AM, Paul Sexton <psexton.2a@gmail.com> wrote:

> Hi, I have just pushed the suggested change to the org-player repository on
> bitbucket. Please let me know if it has fixed the problem.

org-player.el 1.0.1 plays again for links with a position. For links
without position there is a change missing, see my attached patch as a
suggestion.

Michael

[-- Attachment #2: org-player.el.patch --]
[-- Type: text/x-patch, Size: 710 bytes --]

--- org-player.1.0.1.el	2016-01-29 21:32:18.000000000 +0100
+++ org-player.1.0.2.el	2016-02-06 09:27:14.000000000 +0100
@@ -2,7 +2,7 @@
 ;;; org-player.el - Play audio and video files in org-mode hyperlinks
 ;;;
 ;;; Author: Paul Sexton <eeeickythump@gmail.com>
-;;; Version: 1.0.1
+;;; Version: 1.0.2
 ;;; Repository at http://bitbucket.org/eeeickythump/org-player/
 ;;;
 ;;;
@@ -102,7 +102,7 @@
 
 (add-to-list 'org-file-apps
              (cons (concat org-player-file-extensions-regexp "$")
-                   '(org-player-play-file file)))
+                   (lambda (file link) (org-player-play-file file))))
 
 (add-to-list 'org-file-apps
              (cons (concat org-player-file-extensions-regexp