clsid2 / mpc-hc

Media Player Classic
GNU General Public License v3.0
11.16k stars 493 forks source link

Small Fix to The Dark Theme #1822

Closed LucianAndries closed 1 year ago

LucianAndries commented 2 years ago

Hello.

This is for the person who created the Dark Theme for MPC-HC.

Would you please change the color of the scroll arrow buttons from the subtitle list? Right now they are black on dark grey.............. Seriously? I can barely see them up close, not to mention from 2 meters away while in bed. Even github has lighter buttons in its Dark Mode. That should be a clue...

Anyway, please fix this? It's not urgent, it can come out with the player's next update. As long as it gets fixed.

Thank you.

Here's a screenshot for context:

black arrows

adipose commented 2 years ago

Can you provide a reduced size version of the MKV with so many subtitles?

Thanks

LucianAndries commented 2 years ago

Can you provide a reduced size version of the MKV with so many subtitles?

Thanks

Hello. I don't understand the question. Can you rephrase it?

bibsp commented 2 years ago

http://dl2.dlanzdkasbenshinkoazdlkhabrdaradbzirandrakhtirokogolhaytardarad.xyz/TV%20Series/Money%20Heist%202017/S04/WEB-DL%201080p%20x265/Money.Heist.S04E01.10bit.Dual-Audio.x265.1080p.WEB-DL.6Ch.PSA.mkv

This file has many subs. No need to download. Stream it. Link is not mine. Taken from Here

Edit: This link is very fast. Use it. http://khulnaplex.net/Data/tvshow/english/Money%20Heist/s02/Money%20Heist%20S02E03%20720p%20NF%20WEBRip%20Hindi%20English%20AAC%205.1%20MSubs%20x264%20-%20LOKiHD%20-%20Telly.mkv

LucianAndries commented 2 years ago

Pfffffffffffff, thank god for @bibsp otherwise I wouldn't have known you edited your comment! People, stop editing already read comments, we don't get notifications for that! Also, we don't live on github to get notifications on tab/page, either.

Now, why do you need a small file size????????? :OO The issue is with MPC-HC's Dark Theme skin, NOT with the subtitles. If you got the player after it got a Dark Theme, then you already have the issue. I thought I was specific in the op, oh well. The arrows I pointed just need a color change, is that simple. Well, for the one who made the skin...

ale5000-git commented 2 years ago

@LucianAndries He probably need it to test since the arrow doesn't appear when not needed and videos with so many subtitles are NOT the general case.

LucianAndries commented 2 years ago

He probably need it to test since the arrow doesn't appear when not needed and videos with so many subtitles are NOT the general case.

Still, it's a skin, not a program bug. Change the color to white despite being needed or not. THEM being black on a dark gray was dumb to begin with. No offence. You save the skin then apply it for the next build update. Why do it the hard way?

ale5000-git commented 2 years ago

It isn't an app that support external skins, the dark theme is hardcoded in the app executable, and any change could break things and so need to be tested.

adipose commented 2 years ago

Pfffffffffffff, thank god for @bibsp otherwise I wouldn't have known you edited your comment! People, stop editing already read comments, we don't get notifications for that! Also, we don't live on github to get notifications on tab/page, either.

Now, why do you need a small file size????????? :OO The issue is with MPC-HC's Dark Theme skin, NOT with the subtitles. If you got the player after it got a Dark Theme, then you already have the issue. I thought I was specific in the op, oh well. The arrows I pointed just need a color change, is that simple. Well, for the one who made the skin...

I edited the comment to clarify the wording a bit, but the original ask was fine. The issue doesn't show up unless the number of subs is great enough that the menu would need scrolling...hence the need for a use case.

I noticed your example has many subs but is 2.5 hours long so...you can cut the mkv to be about 10 seconds long but it will still have the subs.

I am happy to fix any theme issues if I can reproduce them. That arrow is not something that I currently am drawing myself. It's just using the native drawing code, because I haven't sub-classed that element. Just so you understand, unless I subclass an aspect of the MFC/win32 drawing code, it just draws according to the standard windows common controls. So some edge cases like this can get missed unless they happen to be seen or noticed by someone.

Still, it's a skin, not a program bug. Change the color to white despite being needed or not. THEM being black on a dark gray was dumb to begin with. No offence. You save the skin then apply it for the next build update. Why do it the hard way?

No "offence" to you either, but you have no idea what you're talking about. This is not a skin. It's a complete re-implementation of the native win32 common control and menu drawing code, as well as custom implementations of all the custom mpc-hc widgets. There is no skin file that says "draw scrolling arrow in black." I will have to determine what the win32 subclassed window that draws the scroller is, find a way to identify when it's present, hook it, and re-implement the draw code from scratch (win32 draw code is closed source), possibly with logic to control for DPI and zoom levels.

LucianAndries commented 2 years ago

Pfffffffffffff, thank god for @bibsp otherwise I wouldn't have known you edited your comment! People, stop editing already read comments, we don't get notifications for that! Also, we don't live on github to get notifications on tab/page, either. Now, why do you need a small file size????????? :OO The issue is with MPC-HC's Dark Theme skin, NOT with the subtitles. If you got the player after it got a Dark Theme, then you already have the issue. I thought I was specific in the op, oh well. The arrows I pointed just need a color change, is that simple. Well, for the one who made the skin...

I edited the comment to clarify the wording a bit, but the original ask was fine. The issue doesn't show up unless the number of subs is great enough that the menu would need scrolling...hence the need for a use case.

I noticed your example has many subs but is 2.5 hours long so...you can cut the mkv to be about 10 seconds long but it will still have the subs.

I am happy to fix any theme issues if I can reproduce them. That arrow is not something that I currently am drawing myself. It's just using the native drawing code, because I haven't sub-classed that element. Just so you understand, unless I subclass an aspect of the MFC/win32 drawing code, it just draws according to the standard windows common controls. So some edge cases like this can get missed unless they happen to be seen or noticed by someone.

Still, it's a skin, not a program bug. Change the color to white despite being needed or not. THEM being black on a dark gray was dumb to begin with. No offence. You save the skin then apply it for the next build update. Why do it the hard way?

No "offence" to you either, but you have no idea what you're talking about. This is not a skin. It's a complete re-implementation of the native win32 common control and menu drawing code, as well as custom implementations of all the custom mpc-hc widgets. There is no skin file that says "draw scrolling arrow in black." I will have to determine what the win32 subclassed window that draws the scroller is, find a way to identify when it's present, hook it, and re-implement the draw code from scratch (win32 draw code is closed source), possibly with logic to control for DPI and zoom levels.

Ok, makes sense. Sorry about that, I didn't mean to be rude! You asked for a 10 sec file, I made it 14 sec one, but it's 6mb... Here's a link to Google Drive: https://drive.google.com/file/d/1lfO5EYLYJY6yrLAOTDd5PfuduXYeCrcz/view?usp=sharing

adipose commented 2 years ago

I was able to reproduce it, but it's not enough to scroll in 200% zoom...had to go to 300% zoom. I'll take a look.

LucianAndries commented 2 years ago

I was able to reproduce it, but it's not enough to scroll in 200% zoom...had to go to 300% zoom. I'll take a look.

Well my TV is 1080p, and it's enough to scroll... lol I'm poor....................... :( But glad it helped. It's easier to just make 20 copies of a sub and just merge them to a random mkv, than look for a webrip with enough subs, imo...

adipose commented 2 years ago

Yeah, I can do it at 300x but visual studio is kind of hard to use.

LucianAndries commented 2 years ago

Visual Studio? Pffff, I use MKV Tool Nix GUI... It just merges everything into 1 file. Very easy to use, it's not an editing software. And if you want to extract stuff from a mkv, use gMKV Extract GUI...

adipose commented 2 years ago

Visual Studio is used for updating MPC-HC codebase...

LucianAndries commented 2 years ago

Oooh.... Sounds like an editing software. :)

clsid2 commented 2 years ago

Recent file list can probably be used as alternative to generation a long menu.

adipose commented 2 years ago

I'm not sure why but the recent file list always "fits" for me. Maybe it tries not to grow too large?

LucianAndries commented 2 years ago

Weird, it does not behave as the subs list? I checked on mine and no matter how many files I play, the list shows a maximum of 20 files. And it keeps overwriting the old ones. Maybe because that list is not actually a part of the mkv file, like the subs are.

Out of curiosity, is anyone actually using the recent files list? Focus on the subs list, for now at least...

adipose commented 2 years ago

Weird, it does not behave as the subs list?

I checked on mine and no matter how many files I play, the list shows a maximum of 20 files. And it keeps overwriting the old ones. Maybe because that list is not actually a part of the mkv file, like the subs are.

Out of curiosity, is anyone actually using the recent files list? Focus on the subs list, for now at least...

I actually got it working. Just needed to increase my number of recent files.

LucianAndries commented 2 years ago

Weird, it does not behave as the subs list? I checked on mine and no matter how many files I play, the list shows a maximum of 20 files. And it keeps overwriting the old ones. Maybe because that list is not actually a part of the mkv file, like the subs are. Out of curiosity, is anyone actually using the recent files list? Focus on the subs list, for now at least...

I actually got it working. Just needed to increase my number of recent files.

Hey again.

Can you make another small tweak? Or tweaks?

I appreciate you for making us a Dark Theme!! But making the buttons and indicators black as well, was kind of a bad idea......... You know? Well menu buttons, checkboxes and dropmenus have a light gray outer line which helps a lot when everything is the same color, so I guess those are ok. Oh and the interface buttons are highlighted on mouse over. But the Seek Bar and Volume Button were......neglected. Weeeeeeeelll, if the highlighting buttons color could be a little lighter?๐Ÿค—

So thank you for everything!

clsid2 commented 2 years ago

You must be using the legacy seekbar then. Options > Advanced > ModerSeekbar

The seekbar button should be made lighter yes. Volume already is gray and visible enough here.

LucianAndries commented 2 years ago

You must be using the legacy seekbar then. Options > Advanced > ModerSeekbar

The seekbar button should be made lighter yes. Volume already is gray and visible enough here.

Nope, I'm using the Modern Seekbar, it says True. Well it's not quite black when I come close to the tv, it is a gray but it's not light enough. The volume too.

My setup is a htpc in my room + tv. And at 2.5-3 meters they look black... :( How about make the seekbar and volume this color #535353? Oh, and the blue button which is actually a line, make it 3 times thicker if you can? And for the button highlight color on the interface, use this #4d4d4d.

They are not exaggerated colors from the originals, and they are much easy to see. I tested many shades in Photoshop, so they are not random numbers. And doesn't break the dark theme aesthetic either.

For now I just increased the seekbar's height to 20, so I can see it....................... lol

clsid2 commented 2 years ago

Making the seekbar line thicker is ugly for normal PC users. The current gray color is light enough for me, and my screen brightness is set pretty low. Maybe the brightness of your TV screen is set super low? Maybe you can take a picture? I guess we could make it a little bit lighter. @adipose, what do you think?

The seekbar height setting is there for this purpose. And 20 is still relatively small for large viewing distance.

I am actually considering to change the default from 12 to 16.

LucianAndries commented 2 years ago

Making the seekbar line thicker is ugly for normal PC users. The current gray color is light enough for me, and my screen brightness is set pretty low. Maybe the brightness of your TV screen is set super low? Maybe you can take a picture? I guess we could make it a little bit lighter. @adipose, what do you think?

The seekbar height setting is there for this purpose. And 20 is still relatively small for large viewing distance.

I am actually considering to change the default from 12 to 16.

No no no. I didn't mean the seekbar itself. I mean the small BLUE line that indicates the exact position of the movie. That I need made thicker.

As for the seekbar set to 20 height, I only set that on my end(temporarily) so I can see where I click. I did say the TV was at a distance of 3 meters. I'm using a wireless mouse, yes... Hope that cleared the misunderstanding.

20 is small? Well my TV is 1080p and 46 inches. 20 is ok for MY resolution. If I had a 4K TV, then maybe...

Hmm good idea, I'll take some photos of my TV showing the menus and stuff.

clsid2 commented 2 years ago

I was also talking about the blue line. Thick is ugly.

LucianAndries commented 2 years ago

I was also talking about the blue line. Thick is ugly.

Make it round then...

About the photos, mission failed. My phone just doesn't want to make the photos look like in real life. I disabled any night mode and optimizations, and the photos still look brighter and more saturated. The seekbar is not only visible at size 8, but it's blue-ish instead of gray... Dumb phone!!!!!!!!๐Ÿ˜ญ๐Ÿ˜ญ๐Ÿ˜ญ๐Ÿ˜ญ๐Ÿ˜ญ๐Ÿ˜ก

But try the color values I gave you. They look better(not exaggerated). See for yourselves and decide.

bibsp commented 2 years ago

Color wise, style wise everything is perfect in dark theme. That blue line on seekbar is also perfect. No need to change shape or color. Please don't change anything.

You should set Mordern Seekbar height default value to the lowest. Then the Seekbar becomes exactly same thick like volume slider. Which looks great. ๐Ÿ”ฅ

LucianAndries commented 2 years ago

Ok, for other people that don't want it to change, you can leave IT as is. And in the Advanced settings you can add the option to change it to the one with the colors I need. That way you can put in the colors I gave you, without ruining the Theme that the other people want. And mine will be a version for the "htpc distance viewing" or something...

But the subs list and recent files list fix will need to be on both dark themes. Oh, and the arrows for scrolling can be a slightly bigger. Unless the scrolling can be done with the mouse scroll wheel when we mouse over them?

Another idea for the themes could be, adding the options to choose your own colors for everything, in the Advanced settings.......... Doing this will remove the Themes feature, but let you customize it to look how everyone wants to. Now, I know this is actually way more difficult to do. It was just an idea. I'm trying to please everybody, not just me. Pffff, that sounded wrong... lol

adipose commented 2 years ago

I like the idea of different settings for distance viewing.

Making the seekbar line thicker is ugly for normal PC users. The current gray color is light enough for me, and my screen brightness is set pretty low. Maybe the brightness of your TV screen is set super low? Maybe you can take a picture? I guess we could make it a little bit lighter. @adipose, what do you think?

The seekbar height setting is there for this purpose. And 20 is still relatively small for large viewing distance.

I am actually considering to change the default from 12 to 16.

Yeah I think changing the color is fine. It's one of the things that could be an option, as it's not really part of a cohesive "theme" color but just certain operation colors.

LucianAndries commented 2 years ago

Hey guys, this is Aust--I mean Lucian... lol

Any updates? I don't want to rush anybody, I'm just curious. ^^

adipose commented 2 years ago

I have spent quite a bit of time on the menu issue. I don't have much to report yet. Here's my conclusion so far though.

  1. CMenu, or classic menu, which we currently use, does not have a facility for hooking the scroll buttons. We can find ways to prevent scrolling, like submenus or popups, which is the "right" solution as scrolling menus are usually frowned on by users anyway.

  2. I think cmfcmenubar could do it. I have been investigating using that in mpc. Problem is we have a lot of custom code regarding showing and hiding menus and toolbars, but menubars work differently and may require us to redo the layout. Benefits would be tear off menus, maybe.

clsid2 commented 2 years ago

Using submenus instead of scrolling is probably the best looking solution. Is that possible without switching to cmfcmenubar? Using a different class might be a huge waste of time and possibly give regressions.

Another idea could be to add a "manage recent files" entry at the top that opens a separate window similar to "organize favorites". That can have a scrollbar. Plus there could be a few buttons for management: Open (the selected files), Close (the window) , Remove selected (the selected recent file entries), Remove invalid (removes all no longer existing files).

LucianAndries commented 2 years ago

Wow, I wouldn't imagine in a million years that even the menus of a program can be this complicated....... Or is it because someone else made it? Either way, I respect what you guys do.

adipose commented 2 years ago

Wow, I wouldn't imagine in a million years that even the menus of a program can be this complicated....... Or is it because someone else made it? Either way, I respect what you guys do.

Win32 menus are supposed to be drawn by the OS. Microsoft gives you limited ability to override the menu drawing by listening for the ownerdraw event. It lets you custom draw each row of the menu. But they don't generate an event for the scroll area. Luckily you can set the default background color for a menu so at least that looks right on the scroll area. But actually painting it is 100% controlled by the OS.

If we switched to a newer api we'd have more options.

LucianAndries commented 1 year ago

Hey guys! How are you doing? I hope you're all ok.

Seems this will take a long time, it wasn't in the last update either... Oh well. :)

Sooooo, may I add just a tiny last request? I know I asked a lot in this thread, but I think this really is the last one.

It's about the buttons to increase-decrease playback speed. Now, does anyone actually use this? Well I guess it's good only for when streaming YouTube videos... It has the function, but does anyone watch YT outside of a browser?

What I am trying to say is, replace those buttons with "seek" buttons. Those are more useful for when you want to skip a scene or rewatch a scene. You know, the Seek buttons that go forward-back 10 seconds? Those two. And maybe change in the settings how far(?) they seek, 10 sec, 20 sec or 5 sec. Or you can just add two more buttons, 'cause there's enough space...

What do you say? Please? And of course NOT now, after the new years!

Well thank you anyway, and have a happy new years!!๐ŸŽ‰๐ŸŽ†๐Ÿงจ

clsid2 commented 1 year ago

Request denied.

LucianAndries commented 1 year ago

Request denied.

Ok. But may I ask why though?

Ok, then please do this instead: I managed to set my Logitech mouse to use shortcut key per app specific. The "jump forward-backward" (small) jumps 1 sec, (medium) jumps 5 sec and (large) jumps 20 sec.......... Skipping 1 sec makes no sense, so please change (small) to jump 5 sec and (medium) to jump 10 sec. Or allow us to set it manually. Yes, I think that's way better.

Please? This should be easier than adding some buttons. Even though all modern players have seek buttons now... Eh, it's your project, and you do what you think it's best/right for it. But at least consider this alternative please?

Thank you for your time!

clsid2 commented 1 year ago

You can already change the jump sizes in the options. It is super annoying that people don't even bother looking through all options pages.

You can even seek with Ctrl+MouseScrollwheel.