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

Solve incorrect resolution when dpi is set #126

Closed
wants to merge 7 commits into from

Conversation

HO-COOH
Copy link
Contributor

@HO-COOH HO-COOH commented Feb 4, 2022

I have this maybe over-convoluted approach to solve #76
Also see my comment

But anyway, it does solves the issue.
Here's a screenshot of the result.
image

@rashil2000
Copy link
Member

rashil2000 commented Feb 4, 2022

How did that screenshot look before this change?

Additionally, can you provide some before- and after- performance benchmarks?

You can use Measure-Command in PowerShell (or hyperfine elsewhere).

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 4, 2022

@rashil2000 This is what before was like. I have two 3840x2160 screens scaled at 200%, so it was showing 1920x1080, one at 150% so it was showing 2560x1440, one 2560x1440 and one 1080p both scaled at 100% respectively so they are at native resolution. It was a mess.

image

For performance, this is before

PS C:\Users\Peter\Desktop\winfetch> Measure-Command { .\winfetch.ps1}


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 488
Ticks             : 4882514
TotalDays         : 5.65105787037037E-06
TotalHours        : 0.000135625388888889
TotalMinutes      : 0.00813752333333333
TotalSeconds      : 0.4882514
TotalMilliseconds : 488.2514

After

PS C:\Users\Peter\Desktop\winfetch> Measure-Command { .\winfetch.ps1}


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 674
Ticks             : 6742181
TotalDays         : 7.80345023148148E-06
TotalHours        : 0.000187282805555556
TotalMinutes      : 0.0112369683333333
TotalSeconds      : 0.6742181
TotalMilliseconds : 674.2181

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 4, 2022

Btw, with this approach I can easily add refresh rate to it as #116

@rashil2000
Copy link
Member

rashil2000 commented Feb 6, 2022

Can you merge with latest master? Your branch is very behind. (I did some refactoring to make this PR easier to review).

@rashil2000
Copy link
Member

rashil2000 commented Feb 17, 2022

@HO-COOH any updates on this and #127?

We are planning to make a release. It would be good to include this fix in it.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 18, 2022

Have been too busy at work lately. I will look into that this weekend and get it resolved.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 18, 2022

I merged with master, but I am not able to run it because of the microsoft logo starting at this line,
https://github.com/kiedtl/winfetch/blob/f19207d4f7c2e3d5f18a62239758c6e199511409/winfetch.ps1#L459

image

@rashil2000
Copy link
Member

Looks like your editor messed up some ASCII characters. Could you please open the PR from a fresh branch (created from latest master)?

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 18, 2022

It's not an issue of my editor. I cloned this project, run it fresh, it's producing the error
image

@rashil2000
Copy link
Member

image

@rashil2000
Copy link
Member

What is your $OutputEncoding? And PowerShell version?

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 19, 2022

What is your $OutputEncoding? And PowerShell version?

image

Windows 11, English(US) version

@rashil2000
Copy link
Member

There is a bug in Windows PowerShell version 5.1. To work around this, you can put this line in your $PROFILE:

if ($PSVersionTable.PSVersion.Major -le 5) { $OutputEncoding = [System.Text.Encoding]::UTF8 }

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 20, 2022

image

Nope, still the same. But maybe this should be created as a separate issue, and you can merge this PR first.

@rashil2000
Copy link
Member

Have you tried cloning the repo and running it in PowerShell 7+?

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 21, 2022

@rashil2000 Just tried. It runs fine on Powershell 7, but I would suggest to make it compatible with the powershell 5.1 that comes preinstalled.

@rashil2000
Copy link
Member

It does work on PowerShell 5.1 😅 (I do all my winfetch work on 5.1).

As I said, there are some bugs in PS 5.1 regarding codepages/encoding that you need to workaround yourself (depending on region/language). It is unlikely that these will be fixed given that 5.1 receives no updates.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 21, 2022

I tried it on another PC that has Windows 11, Simplified Chinese fresh installed, it also doesn't run in Powershell 5.1.🤔

@rashil2000
Copy link
Member

Simplified Chinese

Likely that PS 5.1 has issues with this language. If you can't solve it through changing codepage/encoding, I'm sorry there's nothing I or winfetch can do about it. Your only solution is to use a better and more up to date PS 7.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 21, 2022

The thing is I am having issue with the US version, and that's likely to bring issues with quite a lot of users. The thing you can do is remove that Microsoft fallback logo, that's the reason my PR was originally based on the released 2.3 branch.

@rashil2000
Copy link
Member

The logo had been requested by many people for a long time, that's why it was added. I'm reluctant to remove it based on a single report yet.

@jcwillox any thoughts?

In the meanwhile, let's not stall this PR because it's unrelated here. I'd recommend you to use PS 7 to continue with the review here. For the encoding problem, please create a new issue.

@HO-COOH
Copy link
Contributor Author

HO-COOH commented Feb 21, 2022

Yeah ok, other than this, my PR runs fine on Powershell 7.

@rashil2000
Copy link
Member

Could you please open the PR from a fresh branch (created from latest master)?

Because there are a lot of (unrelated) changes in the current PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants