yuchen-lea / org-media-note

Taking interactive notes when watching videos or listening to audios in org-mode.
GNU General Public License v3.0
242 stars 35 forks source link

Consider transitioning from `pretty-hydra` to `transient` #56

Closed leafarbelm closed 4 months ago

leafarbelm commented 5 months ago

Would you consider transitioning pretty-hidra to use transient since it's built with Emacs? This would eliminate one dependency and support all the existing features. I'm currently utilizing this if you are interested:

(transient-define-prefix org-media-note-import-transient ()
    "Transient for org-media-note import commands."
    ["Import"
     [("p" "From pbf" org-media-note-insert-note-from-pbf)
      ("n" "From Noted" org-media-note-insert-note-from-noted)
      ("t" "From org-timer" org-media-note-convert-from-org-timer)
      ("s" "From subtitle" org-media-note-insert-note-from-subtitle)
      ("c" "From chapters" org-media-note-insert-note-from-chapter-list)]])

  (transient-define-prefix org-media-note-config-transient ()
    "Transient for org-media-note config commands."
    ["Config"
     [("m" "Auto insert media item" toggle-org-media-note-auto-insert-item)
      ("s" "Auto insert screenshot" org-media-note-toggle-save-screenshot)
      ("S" "Screenshot with sub" org-media-note-toggle-screenshot-with-sub)
      ("l" org-media-note-config-ab-loop-capture-method
       :description (lambda ()
                      (format "AB-loop Clip: %s"
                              (if org-media-note-capture-ab-loop-ask-each-time
                                  "always ask" org-media-note-default-capture-ab-loop-function-name))))
      ("c" "Cite key instead of path" org-media-note-toggle-refcite)
      ("p" "Pause after insert link" org-media-note-toggle-pause-after-insertion)
      ("t" org-media-note-toggle-timestamp-pattern
       :description (lambda ()
                      (format "Timestamp format: %s"
                              (cond
                               ((eq org-media-note-timestamp-pattern 'hms) "hh:mm:ss")
                               ((eq org-media-note-timestamp-pattern 'hmsf) "hh:mm:ss.fff")))))
      ("M" org-media-note-set-separator
       :description (lambda ()
                      (format "Separator when merge: %s" org-media-note-separator-when-merge)))
      ("<right>" org-media-note-set-seek-method
       :description (lambda ()
                      (format "Seek step: %s" (org-media-note--seek-step t))))]])

  ;; Define the main transient with the sub-transients
  (transient-define-prefix org-media-note-transient ()
    "Main transient for org-media-note."
    :transient-suffix 'transient--do-stay
    [:description org-media-note--hydra-title
                  ["\nFile"
                   ("o" "Open link" org-media-note-play-smart)
                   ("j" "Toggle subtitles" (lambda () (interactive) (mpv-cycle-property "sub")))
                   ("T" "Toggle ontop" (lambda () (interactive) (mpv-cycle-property "ontop")))
                   ("c" "Increase speed" (lambda () (interactive) (org-media-note-change-speed-by 0.1)))
                   ("x" "Decrease speed" (lambda () (interactive) (org-media-note-change-speed-by -0.1)))
                   ("z" "Reset speed" org-media-note-mpv-toggle-speed)]

                  ["Playback"
                   ("<SPC>" "Play/Pause" mpv-pause)
                   ("l" (lambda () (interactive) (mpv-run-command "ab-loop"))
                    :description (lambda ()
                                   (let ((time-a (mpv-get-property "ab-loop-a"))
                                         (time-b (mpv-get-property "ab-loop-b")))
                                     (if (org-media-note--ab-loop-p)
                                         (format "Clear AB-loop (%s-%s)"
                                                 (org-media-note--seconds-to-timestamp time-a)
                                                 (org-media-note--seconds-to-timestamp time-b))
                                       (if (numberp time-a)
                                           (format "Set B of AB-loop (%s-)"
                                                   (org-media-note--seconds-to-timestamp time-a))
                                         "Set A of AB-loop")))))
                   ("g" "Jump to timestamp" org-media-note-goto-timestamp)
                   ("<left>" (lambda () (interactive) (org-media-note-seek 'backward)) :description (lambda () (format "Backward %s" (org-media-note--seek-step t))))
                   ("<right>" (lambda () (interactive) (org-media-note-seek 'forward)) :description (lambda () (format "Forward %s" (org-media-note--seek-step) t)))
                   ("C-<left>" "Previous subtitle" (lambda () (interactive) (mpv-run-command "sub-seek" -1)))
                   ("C-<right>" "Next subtitle" (lambda () (interactive) (mpv-run-command "sub-seek" 1)))
                   ("<prior>" "Previous Chapter" (lambda () (interactive) (mpv-run-command "add" "chapter" -1)))
                   ("<next>" "Next Chapter" (lambda () (interactive) (mpv-run-command "add" "chapter" 1)))]

                  ["Note"
                   ("i" "Insert timestamp" org-media-note-insert-link :transient nil)
                   ("a" "Adjust timestamp" org-media-note-adjust-timestamp-offset)
                   ("S" (lambda () (interactive)
                          (if (org-media-note--ab-loop-p)
                              (org-media-note-capture-ab-loop-and-insert)
                            (org-media-note-insert-screenshot)))
                    :description (lambda ()
                                   (if (org-media-note--ab-loop-p)
                                       "Insert ab-loop clip"
                                     "Insert Screenshot")))
                   ("s" "Insert subtitle" org-media-note-insert-sub-text)
                   ("H-m" "Merge items" org-media-note-merge-item)]

                  ["Volume"
                   ("+" "Up" (lambda () (interactive) (org-media-note-change-volume-by 5)))
                   ("-" "Down" (lambda () (interactive) (org-media-note-change-volume-by -5)))
                   ("0" "Toggle" org-media-note-mpv-toggle-volume)
                   ("m" "(un)mute" (lambda () (interactive) (mpv-cycle-property "mute")))]]

    ["Other"
     [("I" "Import" org-media-note-import-transient)
      ("C" "Config" org-media-note-config-transient)]
     ])
yuchen-lea commented 5 months ago

Thank you very much for your suggestions and demo code, I didn't know transient was built with Emacs!

I'm not very familiar with transient yet, I need to take some time to consider this. Especially when modifying the configuration, such as modifying org-media-note-timestamp-pattern, how to keep the panel open and display the current org-media-note-timestamp-pattern in real time.

If the functionality and interaction logic are OK under transient, I'll switch to transient (or keep hydra as a candidate?)

yuchen-lea commented 5 months ago

@leafarbelm Your demo code was extremely helpful!

Because the UI layout logic of transient is somewhat different, I made some adjustments to the menu:

图片

Currently, the only issue lies in the execution of the ta, tM, and tl commands. They all involve the selection in the minibuffer, which works fine under hydra but causes errors under transient:

⛔ Warning (org-element): ‘org-element-at-point’ cannot be used in non-Org buffer #<buffer Minibuf-1> (minibuffer-mode)

If you have any suggestions for this issue, please let me know! After resolving this issue, we can switch from hydra to transient.

yuchen-lea commented 5 months ago

Minimal reproducible code:

  (transient-define-prefix tsc-interactive-basic ()
    "Prefix with interactive user input."
    :transient-suffix 'transient--do-stay
    [:description org-media-note--hydra-title
     ("ta" "seek step" org-media-note-set-seek-method)
     ]
)
enometh commented 5 months ago

Hello, I have had a full interface to transient at thedefault branchmadhu' under contrib/omn-transient in https://github.com/enometh/org-media-note/blob/madhu/contrib/omn-transient.el at https://github.com/enometh/org-media-note This is a series of patches on top of your master branch - I tried to implement the full interface of pretty-hydra and hydra on it, perhaps you could give this branch a try -- it needs review.

(My sample config for use with org-ref and org-ref-publish is on another branch in the same repository, https://github.com/enometh/org-media-note/blob/org-ref-publish-elisp-config/config-omn.el which shows the keybindings I use to invoke it)

Please let me know if you have comments, I've spent some time on the branch and would appreciate it if the commits could be reviewed and upsstreamed, so I have less work on rebases.

leafarbelm commented 5 months ago

Hi @yuchen-lea! sorry the late reply!

I didn't know transient was built with Emacs!

Yeah, its kind new, it in has been built into emacs since 28.

If the functionality and interaction logic are OK under transient, I'll switch to transient (or keep hydra as a candidate?)

Personally, I would keep only transient since it's built-in and is one less dependency. However, since hydra is already developed and some people may prefer it, you could add it as an optional dependency, maybe?

@leafarbelm Your demo code was extremely helpful!

I'm very happy to hear that!

Minimal reproducible code

I tried this example but didn't get any errors. This is what I get: Captura de tela de 2024-06-19 21-20-05 The when i select one of the itens it opens to enter the value: Captura de tela de 2024-06-19 21-20-25

If you could give more information on how to reproduce the errors, I could try to test more. 😊

leafarbelm commented 5 months ago

Hello, I have had a full interface to transient at thedefault branchmadhu' under contrib/omn-transient in https://github.com/enometh/org-media-note/blob/madhu/contrib/omn-transient.el at https://github.com/enometh/org-media-note This is a series of patches on top of your master branch - I tried to implement the full interface of pretty-hydra and hydra on it, perhaps you could give this branch a try -- it needs review.

(My sample config for use with org-ref and org-ref-publish is on another branch in the same repository, https://github.com/enometh/org-media-note/blob/org-ref-publish-elisp-config/config-omn.el which shows the keybindings I use to invoke it)

Please let me know if you have comments, I've spent some time on the branch and would appreciate it if the commits could be reviewed and upsstreamed, so I have less work on rebases.

Hi @enometh,

I tried your omn-transient.el, and as far as I can tell, it's all working well. I'll use your branch for a few days to test it out more thoroughly.

yuchen-lea commented 4 months ago

Thank @leafarbelm and @enometh for the helpful demo code!

Compared to displaying in a window (transient), I prefer displaying in a frame (hydra), as it provides a larger visual space. Therefore, I have retained pretty-hydra and introduced a new variable, org-media-note-interface, to set which UI to use.

It works fine for me, and if there are no issues reported in a few days, I will merge it (https://github.com/yuchen-lea/org-media-note/tree/transient) into the master branch.

yuchen-lea commented 4 months ago

The logic of transient is slightly different from that of hydra.

When pressing a shortcut key on the emasc transient interface, if the shortcut key is not in the transient prefix, transient will raise a warning.

hydra will just close the window to execute the command.

I like the way hydra handles it. Because I often use M-RET to insert new items after controlling media playback in the UI interface. Closing the UI manually is a distraction.

I'm not sure the preference of others and whether this logic can be implemented in transient

leafarbelm commented 4 months ago

Hi @yuchen-lea,

Sorry for the late reply; I haven't had much time lately. Just wanted to say thanks for the changes you made! I just tested it, and it's working great!