MediaArea / MediaInfo

Convenient unified display of the most relevant technical and tag data for video and audio files.
https://MediaArea.net/MediaInfo
BSD 2-Clause "Simplified" License
1.31k stars 158 forks source link

24.06 - "Allgemein" is cut off if HTML is selected -> scaling @200% #890

Closed Klaus1189 closed 2 months ago

Klaus1189 commented 2 months ago

"Allgemein" is cut off if HTML is selected -> scaling @200% See in this screenshot: cut off 24 06

Klaus1189 commented 2 months ago

Since I already have some expericence because I do translation for MPC-BE, MSI Afterburner, RTSS, Icaros and XnViewMP, I can join help german translation work if you want.

JeromeMartinez commented 2 months ago

@cjee21 could you check?

Klaus1189 commented 2 months ago

If I look closer some more entries are cut off below

cjee21 commented 2 months ago

Thanks for reporting, partially reproducible after the new WebView2 in-progress PR when falling back to IE engine. I'll do more tests and improve it later. Update: Updated both HTML PRs.

I can join help german translation work if you want.

I think the developers will like that. Get started here: https://github.com/MediaArea/MediaInfo/tree/master/Contrib/TranslationToolkit

cjee21 commented 2 months ago

Test with latest PRs...

Screenshot 2024-06-29 174421

In the worst case scenario: IE Edge
Screenshot 2024-06-29 174538 Screenshot 2024-06-29 175529

Edge WebView2 better supports high-DPI.

JeromeMartinez commented 2 months ago

development snapshot, @Klaus1189 please test.

cjee21 commented 2 months ago

development snapshot, @Klaus1189 please test.

This doesn't include the MediaInfoLib PR 2084?

JeromeMartinez commented 2 months ago

This doesn't include the MediaInfoLib PR 2084?

Oops, I relaunch.

cjee21 commented 2 months ago

This doesn't include the MediaInfoLib PR 2084?

Oops, I relaunch.

Need the HTML injections from MediaInfo PR 873 too.

cjee21 commented 2 months ago

FYI my screenshots are taken with all my PRs (including non-HTML-related ones) from MediaInfo and MediaInfoLib merged. I tested to ensure all PRs work well together.

JeromeMartinez commented 2 months ago

Need the HTML injections from MediaInfo PR 873 too.

873 can not yet be merged/tested as is, please extract all not related to EdgeView in 892.

cjee21 commented 2 months ago

873 can not yet be merged/tested as is, please extract all not related to EdgeView in 892.

They are all related except the first one that shouldn't cause issues. The last one depends on if 64-bit is merged first or not. I can remove first one since that is now in 892. The last one I can temporarily remove for now so that you can build but that is needed to get WebView2 working. Updated the last commit to be based on current main branch.

Update: @JeromeMartinez done

JeromeMartinez commented 2 months ago

dev snapshot with up to date https://github.com/MediaArea/MediaInfoLib/pull/2084 and https://github.com/MediaArea/MediaInfo/pull/892.

cjee21 commented 2 months ago

dev snapshot with up to date MediaArea/MediaInfoLib#2084 and #892.

Oh wait, you are using 892 for this? I thought using 873 so I made that build-able and merge-able... If you meant move the HTML injections for IE to 892, the code for both engines are in the same function in the same commit so it's difficult to split...

Just tested, confirmed HTML view is even more broken in that snapshot since 892 does not contain any HTML changes.

JeromeMartinez commented 2 months ago

I thought using 873 so I made that build-able and merge-able...

I won't merge any change to .bat, .nsi, WebView2 related stuff until I test them more deeply.

If you meant move the HTML injections for IE to 892, the code for both engines are in the same function in the same commit so it's difficult to split...

I don't see why a 2nd engine is mandatory for the HTML changes, the code impacting the display should be independent of a 2nd engine integration. If it is not independent it is another reason for me to not merge 873., too complicated.

Just tested, confirmed HTML view is even more broken in that snapshot since 892 does not contain any HTML changes.

It seems better to me but I don't have 200% DPI so I can not test all.


Really, the 2nd web engine should be a totally separate topic, as well as 64-bit stuff, I am fully aware that the current code is ugly but replacing it by something inter-dependent would be even more difficult later.

There should be a way to have all HTML impacting code separate from the count of HTML engines, all HTML only code which does impact the old engine should be able to be there without the new engine presence.

cjee21 commented 2 months ago

I don't see why a 2nd engine is mandatory for the HTML changes, the code impacting the display should be independent of a 2nd engine integration. If it is not independent it is another reason for me to not merge 873., too complicated.

It is because I re-wrote the function to inject HTML styles and used the same function for both engines. Then I only made one commit with all changes. If you want split commits, I can do it but additional work.

It seems better to me but I don't have 200% DPI so I can not test all.

No borders and inconsistent column widths here on 100%.

So to be clear, do you want to split all code necessary for handling MediaInfoLib 2084 + IE Engine to a separate commit and put in 892, then change 873 to be built on-top of 892?

JeromeMartinez commented 2 months ago

So to be clear, do you want to split all code necessary for handling MediaInfoLib 2084 + IE Engine to a separate commit and put in 892, then change 873 to be built on-top of 892?

More or less that. The goal is to fix this issue without handling the 2nd engine, and we slowly manage the 2nd engine independently. So 892, maybe another PR about HTML changes, will be merged first, then I review code related to the 2nd engine without HTML change (except the ones needed for the 2nd engine) separately.

If too much work, we freeze the current work on HTML fixes until I review 873.

cjee21 commented 2 months ago

If too much work, we freeze the current work on HTML fixes until I review 873.

I will see how much work it requires. Looking at the cpp now.

cjee21 commented 2 months ago

@JeromeMartinez done putting it in 892. This part is not that difficult. More difficult will be cleaning up 873 later I think. So you will prefer if I split 873 into smaller easier to understand commits?

Klaus1189 commented 2 months ago

development snapshot, @Klaus1189 please test.

This one does not work, same issue.

dev snapshot with up to date MediaArea/MediaInfoLib#2084 and #892.

This one works, but a small issue: https://drive.google.com/file/d/1vheaKZ8ZZPDT-3oVP0wlWRL01WV1w4U1/view?usp=sharing

the bottom lines (Bild) are more to the right, or the top lines (Allgemein) are more to the left. I think it would be best to be in one line

Klaus1189 commented 2 months ago

I must also add that 200% scaling looks so much fresher than older versions. Some things to tweak and it looks perfect

JeromeMartinez commented 2 months ago

new dev snapshot @cjee21 @Klaus1189.

cjee21 commented 2 months ago

@Klaus1189 please also confirm if it satisfactorily resolves your issue 889.

JeromeMartinez commented 2 months ago

So you will prefer if I split 873 into smaller easier to understand commits?

Should be small enough after rebase.

cjee21 commented 2 months ago

new dev snapshot @cjee21 @Klaus1189.

It's a pass/OK for me.

Klaus1189 commented 2 months ago

Now it is fine for me, Allgemein is not cut off, so this issue 890 is solved.

But it is now fully black, before it was not: https://drive.google.com/file/d/1unuC31d1KGu5ZOSkis6kYusBUn9Onc0v/view?usp=sharing I don't know what will be easier to read. Here an old not fully black, more greyish: https://drive.google.com/file/d/1vheaKZ8ZZPDT-3oVP0wlWRL01WV1w4U1/view?usp=sharing

889 not fully solved, still a bit off: https://drive.google.com/file/d/1rK2Vr8qL4l96aIYgbdDlxjvF22tku_08/view?usp=sharing

Klaus1189 commented 2 months ago

The more I think about, I think the grey is more pleasing for the eyes. What do you think?

Klaus1189 commented 2 months ago

889 is not fully solved

JeromeMartinez commented 2 months ago

But it is now fully black, before it was not [...] The more I think about, I think the grey is more pleasing for the eyes. What do you think?

@cjee21 I think I also prefer the old greyish color, could you put it back in another PR. I already merged the related PRs so need to create a new one.

cjee21 commented 2 months ago

Now it is fine for me, Allgemein is not cut off, so this issue 890 is solved.

Good.

But it is now fully black, before it was not: https://drive.google.com/file/d/1unuC31d1KGu5ZOSkis6kYusBUn9Onc0v/view?usp=sharing

Intended. It is not fully black but now follow Edge browser's dark theme colour because planning to implement Edge WebView2 in the future and WebView2 has native dark theme which does not require manually selecting the background/font colours.

889 not fully solved, still a bit off: https://drive.google.com/file/d/1rK2Vr8qL4l96aIYgbdDlxjvF22tku_08/view?usp=sharing

I don't think it is possible to get exact match with the current state of the codes. It is perfect on 100% DPI.

Klaus1189 commented 2 months ago

Bummer.

JeromeMartinez commented 2 months ago

It is not fully black but now follow Edge browser's dark theme colour because planning to implement Edge WebView2 in the future and WebView2 has native dark theme which does not require manually selecting the background/font colours.

Isn't it possible to force grey background for the older HTML engine?

JeromeMartinez commented 2 months ago

I don't think it is possible to get exact match with the current state of the codes. It is perfect on 100% DPI.

Seems not: image

Something I can fix, I think, looks like just positioning issue, @cjee21 if you like to play to try I let you :-p else I'll check that later.

cjee21 commented 2 months ago

Isn't it possible to force grey background for the older HTML engine?

If you want the inconsistency between the two engines, just change this line: https://github.com/MediaArea/MediaInfo/blob/dda61d6a99aa58fbf69b488669c2ac26242222b2/Source/GUI/VCL/GUI_Main.cpp#L164

You can change background-color: #121212; color: #FFFFFF; to any background and font colour you like 😉

cjee21 commented 2 months ago

Something I can fix, I think, looks like just positioning issue, @cjee21 if you like to play to try I let you :-p else I'll check that later.

Screenshot 2024-06-30 005020

See it's perfect here. That is why it is hard to fix. Need to test light, dark, different DPI. I'm not going to do this. One or 2 pixels off there is not important in my opinion and how often does that window get used anyway.

JeromeMartinez commented 2 months ago

If you want the inconsistency between the two engines

I am not sure to understand. My understanding is that we need to have a fix color for the older engine and that for the new engine it will be from the selected theme, isn't it?

You can change background-color: #121212; color: #FFFFFF; to any background and font colour you like 😉

What was the older value?

JeromeMartinez commented 2 months ago

See it's perfect here.

That on my side: image

I'm not going to do this.

Totally legitimate, I'll also see if it takes time or not. Thank you again for you hard work.

cjee21 commented 2 months ago

If you want the inconsistency between the two engines

I am not sure to understand. My understanding is that we need to have a fix color for the older engine and that for the new engine it will be from the selected theme, isn't it?

The new engine uses #121212 as background when in dark theme (this has nothing to do with VCL themes and is the same as Edge browser when you switch Windows to dark scheme). So I used that for old engine too for consistency, so that whatever engine is used, the display is as similar (other than Edge handling DPI scaling of column widths better) as possible.

You can change background-color: #121212; color: #FFFFFF; to any background and font colour you like 😉

What was the older value?

Here: https://github.com/MediaArea/MediaInfo/blob/d5e8e94d3c3c67c68c041d1d3ab3ac5832eb29cf/Source/GUI/VCL/GUI_Main.cpp#L150

Totally legitimate, I'll also see if it takes time or not. Thank you again for you hard work.

Furthermore, is has been more obviously off for years isn't it?

JeromeMartinez commented 2 months ago

So I used that for old engine too for consistency, so that whatever engine is used, the display is as similar (other than Edge handling DPI scaling of column widths better) as possible.

Good argument. Hesitating. On one hand greyish thing is common, on another hand I may prefer to follow Microsoft choice instead of changing.

I have no idea about what is best. If I check GitHub on Edge, I see a difference: image Ours is darker. What do you think about using GitHub background color for all engines?

Actually, about this sentence:

is the same as Edge browser when you switch Windows to dark scheme

I don't catch it, where is this dark color in practice?

Furthermore, is has been more obviously off for years isn't it?

Yes, but as someone notices it... (and this reasoning is also for highDPI and you were motivated to fix that ;-) ).

Again, not a criticism, you are the one who decides what is important enough for you for sending any patch.

cjee21 commented 2 months ago

@JeromeMartinez The old value is matching the text view background.

Side note: We might get complains from OLED screen users next about how the dark theme is not close to completely black and causing battery drain and screen aging 😆

JeromeMartinez commented 2 months ago

Side note: We might get complains from OLED screen users next about how the dark theme is not close to completely black and causing battery drain and screen aging 😆

An option! :-p (joking)

JeromeMartinez commented 2 months ago

The old value is matching the text view background.

What about using it for both engines?

cjee21 commented 2 months ago

Actually, about this sentence:

is the same as Edge browser when you switch Windows to dark scheme

I don't catch it, where is this dark color in practice?

Export HTML from the new version, then open the HTML in Edge, then change Windows to dark mode. That's the default dark background. You can see there are no colour codes in the HTML source code.

cjee21 commented 2 months ago

@JeromeMartinez about the Qt version:

This is how it looks after recent HTML changes: Before After
Screenshot 2024-06-20 124735 Screenshot 2024-06-30 105703

If it is desired to have proper HTML rendering and identical display as VCL version....

The good news is I have a 4-line patch to achieve the results below: Light Dark
Screenshot 2024-06-30 110049 Screenshot 2024-06-30 110107

The not so good news is that with this patch, Qt will bundle an entire web engine with MediaInfo which greatly increases total size. Therefore, the patch remains on a branch in my fork without a PR.

Note that Qt's dark mode HTML background is also 121212.