Skip to content
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

console.lua tweaks #15036

Merged
merged 3 commits into from
Oct 14, 2024
Merged

console.lua tweaks #15036

merged 3 commits into from
Oct 14, 2024

Conversation

guidocella
Copy link
Contributor

commit 1: console.lua: close with right click

Useful to close the console after it was opened with the mouse.

Also fix the spacing of the previous line.

commit 2: select.lua: print selection circles before playlist entries

Requested in
#14087 (comment) and
#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry after moving the mouse.

@@ -48,6 +48,8 @@ mp.add_key_binding(nil, "select-playlist", function ()
if entry.playing then
default_item = i
end

playlist[i] = (entry.playing and "●" or "○") .. " " .. playlist[i]
Copy link
Contributor

@kasper93 kasper93 Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have color/shadow/border highlight for current item, instead of bloating everything with ● and ○. Note that if we are not in terminal, we can use ASS, else we can use circles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with styles.warn because it's similar to the yellow of the selected item, better suggestions are welcome. The terminal style is just bold.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to the same color as the selected item but not bold.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little bit similar looking. I was thinking something more subtle like "hidden items" color. But it is fine, I'm not so much into colors, I can get used to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @na-na-hi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? I'm talking about playlists shown with g-p which don't have circles. Tracks shown with g-a etc do have.

Copy link
Contributor

@kasper93 kasper93 Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why two completely separate views has to have the same representation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that g-t has multiple selected tracks and also g-s and g-S with secondary subtitles (also audio and video with --lavfi-complex but changing aid and vid doesn't work in that case anyway), so highlighting the default item is not enough. Actually with this change you can differentiate whether a selected subtitle is primary or secondary depending on whether you pressed g-s or g-S which is nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why two completely separate views has to have the same representation?

They are the same for the "native" playlist and track list properties. I don't see how adding circles in the playlist is an issue, but the current solution can stay if you insist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added circles only to tracks precisely because multiple tracks can be selected but not multiple playlist entries or anything else, it is not to arbitrarily make separate views different or whatever.

@guidocella guidocella force-pushed the console-tweaks branch 3 times, most recently from 81c5635 to bc71d24 Compare October 9, 2024 19:35
@Samillion
Copy link
Contributor

Samillion commented Oct 10, 2024

Would it be viable to add a user_opt to control output direction?

For example: top, bottom (bottom being default, as is now).

Where top just simply adjusts the margin so it would act like a regular terminal output (downwards instead of upwards).

@guidocella
Copy link
Contributor Author

Shells also output above the input line, I think that what you mean is to start from the top of the screen and descend as text is outputted, something like this:

diff --git a/player/lua/console.lua b/player/lua/console.lua
index 4e2a942cca..8128a82da3 100644
--- a/player/lua/console.lua
+++ b/player/lua/console.lua
@@ -563,24 +563,24 @@ local function update()
     end
 
     ass:new_event()
-    ass:an(1)
-    ass:pos(bottom_left_margin, screeny - bottom_left_margin - global_margins.b * screeny)
+    ass:an(7)
+    ass:pos(bottom_left_margin, bottom_left_margin + global_margins.t * screeny)
     ass:append(log_ass .. '\\N')
-    if #suggestions > 0 then
-        ass:append(suggestion_ass .. '\\N')
-    end
     ass:append(style .. ass_escape(prompt) .. ' ' .. before_cur)
     ass:append(cglyph)
     ass:append(style .. after_cur)
+    if #suggestions > 0 then
+        ass:append('\\N' .. suggestion_ass)
+    end
 
     -- Redraw the cursor with the REPL text invisible. This will make the
     -- cursor appear in front of the text.
-    ass:new_event()
-    ass:an(1)
-    ass:pos(bottom_left_margin, screeny - bottom_left_margin - global_margins.b * screeny)
-    ass:append(style .. '{\\alpha&HFF&}' .. ass_escape(prompt) .. ' ' .. before_cur)
-    ass:append(cglyph)
-    ass:append(style .. '{\\alpha&HFF&}' .. after_cur)
+    -- ass:new_event()
+    -- ass:an(1)
+    -- ass:pos(bottom_left_margin, screeny - bottom_left_margin - global_margins.b * screeny)
+    -- ass:append(style .. '{\\alpha&HFF&}' .. ass_escape(prompt) .. ' ' .. before_cur)
+    -- ass:append(cglyph)
+    -- ass:append(style .. '{\\alpha&HFF&}' .. after_cur)
 
     mp.set_osd_ass(screenx, screeny, ass.text)
 end

It is no longer possible to position the redrawn cursor correctly from a new ASS event but it's not noticeable anyway (I actually had to ask rossy what it's for 2 years ago - it's because the previous cursor is drawn below the text when they overlap). The order of completions would have to be completely changed so probably not worth maintaining both as an option. I don't know if we should change this.

@Samillion
Copy link
Contributor

Nah, definitely not worth the hassle. It was merely a thought to throw out there for the sole purpose of "osc is bottom too".

I think select being mainstream just got me too excited and thinking of "they'll ask for this, I know it". But why bother, maybe it'll never be requested. xD

@guidocella
Copy link
Contributor Author

Oh you actually meant to anchor the input line to the top for select. This does it:

diff --git a/player/lua/console.lua b/player/lua/console.lua
index 4e2a942cca..0ff447b446 100644
--- a/player/lua/console.lua
+++ b/player/lua/console.lua
@@ -563,21 +563,21 @@ local function update()
     end

     ass:new_event()
-    ass:an(1)
-    ass:pos(bottom_left_margin, screeny - bottom_left_margin - global_margins.b * screeny)
-    ass:append(log_ass .. '\\N')
-    if #suggestions > 0 then
-        ass:append(suggestion_ass .. '\\N')
-    end
+    ass:an(7)
+    ass:pos(bottom_left_margin, bottom_left_margin + global_margins.t * screeny)
     ass:append(style .. ass_escape(prompt) .. ' ' .. before_cur)
     ass:append(cglyph)
     ass:append(style .. after_cur)
+    if #suggestions > 0 then
+        ass:append('\\N' .. suggestion_ass)
+    end
+    ass:append('\\N' .. log_ass)

     -- Redraw the cursor with the REPL text invisible. This will make the
     -- cursor appear in front of the text.
     ass:new_event()
-    ass:an(1)
-    ass:pos(bottom_left_margin, screeny - bottom_left_margin - global_margins.b * screeny)
+    ass:an(7)
+    ass:pos(bottom_left_margin, bottom_left_margin + global_margins.t * screeny)
     ass:append(style .. '{\\alpha&HFF&}' .. ass_escape(prompt) .. ' ' .. before_cur)
     ass:append(cglyph)
     ass:append(style .. '{\\alpha&HFF&}' .. after_cur)

This is more usable for select since the selected match is near the input line but I don't know if we want to deal with having 2 versions.

@Samillion
Copy link
Contributor

but I don't know if we want to deal with having 2 versions.

Agreed, it will introduce redundancy very quickly, not worth it.

Copy link

github-actions bot commented Oct 10, 2024

Download the artifacts for this pull request:

Windows
macOS

Useful to close the console after it was opened with the mouse.

Also fix the spacing of the previous line.
Requested in
mpv-player#14087 (comment) and
mpv-player#15031 (comment)

This is mainly useful to keep highlighting the current playlist entry
after moving the mouse.
@kasper93
Copy link
Contributor

We are not sure about the color, but it can be adjusted. The rest of the improvements work well.

@kasper93 kasper93 merged commit 661bd61 into mpv-player:master Oct 14, 2024
24 checks passed
@guidocella guidocella deleted the console-tweaks branch October 14, 2024 18:14
guidocella added a commit to guidocella/mpv that referenced this pull request Oct 27, 2024
Make the console easier to read because the current default is too
small. See for example
mpv-player#14903 (reply in thread)
or mpv-player#15036 (comment)
or mpv-player#15145 (comment)
or mpv-player#15031 (comment).

This also prevents libass from decreasing performance by printing many
lines.
kasper93 pushed a commit that referenced this pull request Oct 27, 2024
Make the console easier to read because the current default is too
small. See for example
#14903 (reply in thread)
or #15036 (comment)
or #15145 (comment)
or #15031 (comment).

This also prevents libass from decreasing performance by printing many
lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants