Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FR] #341 #733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SurFace81 wants to merge 1 commit into FoedusProgramme:beta
base: beta
Choose a base branch
Loading
from SurFace81:beta
Open

[FR] #341 #733

SurFace81 wants to merge 1 commit into FoedusProgramme:beta from SurFace81:beta

Conversation

@SurFace81
Copy link
Contributor

@SurFace81 SurFace81 commented Oct 12, 2025
edited
Loading

#341 - Scroll minimized player to change song

Copy link
Member

nift4 commented Oct 12, 2025

Hi, thanks for the PR. I very much appreciate you going through these backlog items I never have time for.
Can you try to rebase the PR instead of merging back beta branch? It got a bit messy because I force pushed to fix a mistake in mailmap (sorry about that).
Also while the fix for #619 is a no-brainer to merge (I've cherry picked it just now to simplify rebasing), I'm not yet sure about swipe to change track in the mini player. Both in general (how I want to react to motion in the app, if I agree with the opinions in the feature request, whether it should be a toggle, whether it should hide normal next button) and specifically about the fact there isn't an animation, but good motion design generally includes animation. Also I didn't look at the code in detail yet but usually for consistent swipe UX in Android you use GestureDetector to detect swipes. Also now that weekend is almost over here I'm gonna be a bit short on time again so I'll need some time to think about swipe on miniplayer in detail and maybe consult with duo.

Copy link
Contributor Author

So I should create a new branch for these changes and close this PR, right?

Copy link
Member

nift4 commented Oct 13, 2025

a PR is always synchronized to a branch while it's open. You can also keep using the same branch by overwriting it with a force push.
if you force push the branch, the PR will be updated. so it's fine to keep PR open (the outdated title/description doesn't matter, and even if it does, it can be edited).

In general you would run:

git fetch https://github.com/FoedusProgramme/Gramophone beta
git reset --hard FETCH_HEAD
git cherry-pick 8106919a160c1774af7a1d78b5f118a476fc411d
git push -f <your remote name / github url> beta

to do a destructive resynchronization with my branch, and then take the commit you made which isn't merged yet.

SurFace81 reacted with thumbs up emoji

Copy link
Contributor Author

Looks like everything’s correct now.

@SurFace81 SurFace81 changed the title (削除) [FR] #341, #619 (削除ここまで) (追記) [FR] #341 (追記ここまで) Oct 13, 2025
Copy link
Member

nift4 commented Oct 17, 2025

Hi, thanks again for the PR.

so after thinking it over, I would like there to be an animation that reacts to the drag before this is merged. That is important for discoverability (so people know why the song just changed) and it also just looks better.
Something I have in mind is that dragging/flinging the mini player from the sides makes it slide in drag direction, and where would be empty space is a copy of it slide in from the side, which already contains the song that will be played if the drag is completed.
It's also better to not hide the next button, and maybe don't even offer a toggle for the feature. Instead, it should just be tuned to be hard to trigger accidentally. While I don't 100% agree with the essay, I think https://ometer.com/preferences.html is a good explainer on why less preferences is generally a good thing.

Additionally, please try to use GestureDetector instead of manual MotionEvent handling, it is less error prone.

(If you ping me in an issue before starting to work on it, I could also outline possible considerations before you start, if you want to.)

Copy link
Contributor Author

Hi, I’ll do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /