mmitch / gbsplay

gameboy sound player
https://mmitch.github.io/gbsplay/
Other
98 stars 20 forks source link

Added L and A keys to interface, to loop single / all. Also show loop status on UI. #74

Closed Erhannis closed 2 years ago

Erhannis commented 2 years ago

Often, when I'm listening to music, I'll stop in the middle and just want to loop the current song until I feel like moving on. So I added keys to toggle the loop mode without exiting the program.

So it shows like:

commands:  [p]revious subsong   [n]ext subsong   loop [a]ll    [q]uit player
           [ ] pause/resume   [1-4] mute channel [l]oop single

Song  10/ 30 [Loop single] (Untitled)
00:06/02:00  A-4 %%-   F-3 %%-   C-3 %%%   ---       [ =%%|%%= ]
codecov[bot] commented 2 years ago

Codecov Report

Merging #74 (4777dc1) into master (b15f500) will decrease coverage by 0.41%. The diff coverage is 0.00%.

:exclamation: Current head 4777dc1 differs from pull request most recent head 1d44ed9. Consider uploading reports for the commit 1d44ed9 to get more accurate results

Impacted Files Coverage Δ
gbsplay.c 36.36% <0.00%> (-3.25%) :arrow_down:
player.c 38.98% <0.00%> (-1.44%) :arrow_down:
gbs.c 30.05% <0.00%> (-1.15%) :arrow_down:
cfgparser.c 80.17% <0.00%> (+0.48%) :arrow_up:
mapper.c 48.27% <0.00%> (+5.27%) :arrow_up:
mmitch commented 2 years ago

Good idea!

  1. If you toggle a screen refresh after pause has been toggled, will that actually display the "Pause" status?
  2. What about using the l key to cycle through the loop modes none, single and all? Would that be inconvenient to you?

I think internally we should refactor to a loop enum instead of two booleans anyway because setting both loop modes at the same time makes no sense as discussed here. But I'd do the refactoring after this pull request, we don't have to mix these two things here.

Erhannis commented 2 years ago

Thanks - and:

  1. Well, I tried copying some of the presumable redraw code to just after the toggle_pause call. It seems to work; can you verify there's no weird side effects you expect to occur as a result of the change?
  2. And sure; done.
ranma commented 2 years ago

Do you mind rebasing your pull request against current head? Thanks!

Erhannis commented 2 years ago

Rebasing is pretty weird for me; I prefer merges; but I think I've done it correctly. I've updated my code to deal with the loop mode changes - [Loop all] loops over the range, if one was specified, and everything else seems to behave pretty much as you might expect.

mmitch commented 2 years ago

Sorry for being late. As I see it, with ranmas commit c775085 most if this branch is already in master and the only thing missing is the pause display stuff. Is that correct?

Should I try to cherry-pick the remaining changes? (I don't know if that is possible from a remote repository; also commit 7f9672a contains both the loop handling and the pause mode display when I only need the latter. Commit 3dccade would be cherry-picked as a whole.)

Erhannis commented 2 years ago

I've put the changes into a single commit and made pull request https://github.com/mmitch/gbsplay/pull/79

mmitch commented 2 years ago

Thanks a lot! I've merged #79, so I'm closing this issue.