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

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

Closed
Klaus1189 opened this issue Jun 28, 2024 · 44 comments · Fixed by #892
Closed

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

Klaus1189 opened this issue Jun 28, 2024 · 44 comments · Fixed by #892
Assignees

Comments

@Klaus1189
Copy link

Klaus1189 commented Jun 28, 2024

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

@Klaus1189
Copy link
Author

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
Copy link
Member

@cjee21 could you check?

@Klaus1189
Copy link
Author

If I look closer some more entries are cut off below

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

development snapshot, @Klaus1189 please test.

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

development snapshot, @Klaus1189 please test.

This doesn't include the MediaInfoLib PR 2084?

@JeromeMartinez
Copy link
Member

This doesn't include the MediaInfoLib PR 2084?

Oops, I relaunch.

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

This doesn't include the MediaInfoLib PR 2084?

Oops, I relaunch.

Need the HTML injections from MediaInfo PR 873 too.

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

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

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

@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
Copy link
Author

Klaus1189 commented Jun 29, 2024

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
Copy link
Author

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

@JeromeMartinez
Copy link
Member

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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

@JeromeMartinez
Copy link
Member

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

Should be small enough after rebase.

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

new dev snapshot @cjee21 @Klaus1189.

It's a pass/OK for me.

@JeromeMartinez JeromeMartinez assigned cjee21 and unassigned g-maxime Jun 29, 2024
@Klaus1189
Copy link
Author

Klaus1189 commented Jun 29, 2024

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

@JeromeMartinez JeromeMartinez linked a pull request Jun 29, 2024 that will close this issue
@Klaus1189
Copy link
Author

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

@Klaus1189
Copy link
Author

889 is not fully solved

@JeromeMartinez
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Author

Bummer.

@JeromeMartinez
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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:

StyleContent = L"<meta http-equiv='X-UA-Compatible' content='IE=edge'><style>body { background-color: #121212; color: #FFFFFF; } table { border:1px solid blue; }</style>";

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

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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:

const wchar_t* StyleContent = L"<style>body { background-color: #1F1F1F; color: #FFFFFF; }</style>";

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
Copy link
Member

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
Copy link
Collaborator

cjee21 commented Jun 29, 2024

@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
Copy link
Member

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
Copy link
Member

The old value is matching the text view background.

What about using it for both engines?

@cjee21
Copy link
Collaborator

cjee21 commented Jun 29, 2024

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
Copy link
Collaborator

cjee21 commented Jun 30, 2024

@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.

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 a pull request may close this issue.

4 participants