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

Add basic HiDPI support #48

Merged
merged 3 commits into from Sep 3, 2017
Merged

Add basic HiDPI support #48

merged 3 commits into from Sep 3, 2017

Conversation

tintou
Copy link
Contributor

@tintou tintou commented Jul 15, 2017

No description provided.

@tintou
Copy link
Contributor Author

tintou commented Jul 15, 2017

Note that this doesn't cover all the cases but is a preliminary work on HiDPI

}

int opCmp (const ImageSize s) const
{
if (this.scale != s.scale) {
if (this.scale > s.scale)
return 1;
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look right... That was an 32x32@2 size will be ordered before an 128x128 size. I don't think this has practical implications, but we should likely resolve that so nobody wonders about this in future.

@ximion
Copy link
Owner

ximion commented Jul 16, 2017

Looks pretty good - the tarball generation is missing, but that's trivial to do.

Did you test this and see if icons are correctly generated?

@tintou
Copy link
Contributor Author

tintou commented Jul 17, 2017

Should be fixed, I'll try to generate the files on the elementary PPA once I get some time

@@ -129,6 +136,9 @@ public:

private bool directoryMatchesSize (Algebraic!(int, string)[string] themedir, ImageSize size)
{
int scale = themedir["scale"].get!(int);
Copy link
Owner

Choose a reason for hiding this comment

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

You could make this just themedir["scale"].get!int;, D doesn't need the brackets here (that particular code is rather old from when I didn't know that much about D yet ^^)
But that's just a style-thing, no need to change it.

@@ -320,10 +320,13 @@ public:

// prepare icon-tarball array
immutable iconTarSizes = ["64", "128"];
immutable iconTarScales = ["1", "2"];
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder whether we should iterate over wantedIconSizes here, instead of defining another array (minor thing though).

@ximion
Copy link
Owner

ximion commented Jul 19, 2017

Still looking good, in fact I think this is merge-ready. I'd just like to know whether you tested this meanwhile (some additional data on this is helpful).

@ximion
Copy link
Owner

ximion commented Sep 3, 2017

Hmm, it looks like I forgot to merge this... Sorry for the delay! I'll merge it and resolve a few minor quirks later.

@ximion ximion merged commit 53c122d into ximion:master Sep 3, 2017
@tintou tintou deleted the tintou/hidpi branch September 5, 2017 12:19
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.

None yet

2 participants