search

aome510 / spotify-player

A Spotify player in the terminal with full feature parity
MIT License
3.57k stars 155 forks source link

Sixel output messed #389

Closed 71zenith closed 8 months ago

71zenith commented 8 months ago

Describe the bug When i try to press an keychord such as g it shows the shortcut menu below Playback section. This leads to the sixel image preview getting broken. When the keychord is completed , the playback section goes to its original position but sixel isn't redrawn. I can make it redraw the image forcefully by resizing the terminal.

To Reproduce

  1. have sixel enabled
  2. play a song
  3. press a keychord (e.g. g l)
  4. the image preview is broken

Expected behaviour I believe the solution could be either fixed by making the shortcut menu appear above the playback like pre 0.17.0 or making the screen redraw the image when it is changed

Screenshots If applicable, add screenshots to help explain your problem. image

Environment

aome510 commented 8 months ago

Should be fixed by #390. Can you try running from that branch?

71zenith commented 8 months ago

image It does redraw but leaves some artifacts and the text from before isn't cleared

aome510 commented 8 months ago

image It does redraw but leaves some artifacts and the text from before isn't cleared

I don't have this problem for iterm2. It might happen because sixel doesn't render the image fully in the allocated area. Can you try to tweak cover_img_width, cover_img_length, cover_img_scale to see if those configs fix the issue?

Personally, I don't want to revert to the old popup behaviour in which popups are rendered above the playback (if in bottom) as it looks weird.

71zenith commented 8 months ago

I tried a number of scaling options. But the text is still under the image. Maybe if there is a way to replace the section under image with spaces just like how the rest of playback is overrides the popup text image

aome510 commented 8 months ago

I tried a number of scaling options. But the text is still under the image. Maybe if there is a way to replace the section under image with spaces just like how the rest of playback is overrides the popup text image

Scaling can be set to be >1 FYI, so you can make the image to cover the whole section with text artifact. Can also change length/width of the image rectangle to achieve the best-looking result. The default settings are not tuned for sixel at all as I don't really that feature.

If none of the above options works, I can take another look this weekend to see if there is a better way to fix the underlying issue.

71zenith commented 8 months ago

I was using a scale of 2.2 before. I have managed to cover the text besides the image. But the text below the image still shows. Im assuming this can also be covered if i were to use width and height rather than scale but I'd rather not use that cause it looks out of place without proper scaling

aome510 commented 8 months ago

@71zenith figured out a better way to handle the re-draw. Can you try again with the latest change in https://github.com/aome510/spotify-player/pull/390?

71zenith commented 8 months ago

I tried it but the problem still remains image

aome510 commented 8 months ago

I tried it but the problem still remains image

Hmm super weird. That means Clear doesn’t clear the area.

aome510 commented 8 months ago

Can you share the configs you are using now, e.g what is the width/length of the cover image rectangle, width of the playback window?

aome510 commented 8 months ago

@71zenith , I managed to add some hacks to reproduce your issue with iterm2. Pushed a fix, which I strongly believe should fix the issue.

71zenith commented 8 months ago 
border_type = "Hidden"
client_id = ""
client_port = 8080
cover_img_scale = 2.4
default_device = ""
enable_media_control = true
liked_icon = "ο‚Š"
pause_icon = " "
play_icon = " "
playback_window_position = "Bottom"
progress_bar_type = "Rectangle"
theme = "oxocarbon"
track_table_item_max_len = 32
tracks_playback_limit = 5

[device]
audio_cache = false
bitrate = 320
device_type = "speaker"
name = ""
71zenith commented 8 months ago

@71zenith , I managed to add some hacks to reproduce your issue with iterm2. Pushed a fix, which I strongly believe should fix the issue.

Sadly it still doesnt fix it.

aome510 commented 8 months ago

Does it still have text under the image or different issue with image not fully rendered, which I'm aware of and trying to fix now?

aome510 commented 8 months ago

My last attempt in https://github.com/aome510/spotify-player/pull/390/commits/4f28c2e54056cc1f8ac0185f2d6802386a391359, if this still does not work. I'll revert to the old behaviour (popup above playback for bottom) then 😞 .

71zenith commented 8 months ago

Does it still have text under the image or different issue with image not fully rendered, which I'm aware of and trying to fix now?

Image is rendered correctly but it still has text below it

aome510 commented 8 months ago

Does it still have text under the image or different issue with image not fully rendered, which I'm aware of and trying to fix now?

Image is rendered correctly but it still has text below it

Are u using the latest version of https://github.com/aome510/spotify-player/pull/390?

71zenith commented 8 months ago

I am yes

aome510 commented 8 months ago

Still don't understand why my latest patch didn't work for you πŸ˜•. That said, I reverted the rendering order back to before v0.17.0 so it should work now. Thank you for all the back-and-forth!

71zenith commented 8 months ago

@aome510 i am really really sorry. it looks like i did a build for release and was testing the binary for debug. It works perfectly with your last patch without any artifacts. I should have tested more clearly

aome510 commented 8 months ago

No worries. Glad to hear that I'm not hallucinating. I can push a "revert revert" change xD