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

feat(cli): add option to make custom icon sizes, closes #5121 #5246

Merged
merged 20 commits into from
Dec 27, 2022

Conversation

khuongduy354
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@khuongduy354
Copy link
Contributor Author

Feature requested in this issue. Some keynotes:

  • Only apply to .ico and .png
  • Added --config to find json config file.
  • If no --config provided, use default hardcoded values, as in previous version

@FabianLars
Copy link
Member

I don't think there is any value in making it possible to generate custom .ico files. Just like the .icns file it's basically "complete" and there shouldn't be an actual use case requiring different layer sizes.

It's a good start btw! :)

i'll wait with the code nitpicking until it's more feature complete, but one thing already, you can use serde_json::from_reader instead of from_slice. It's a little cleaner and probablyyy faster.

@FabianLars
Copy link
Member

Hello 👋
Sorry for the super long delay, it thought you weren't done and then forgot about it after some time :/

Do you still have interest in working on it or should we take it from here?

A few open points i see at first glance:

  • It still supports ICO files. As said earlier i don't think this offers much value because the default ico is already "complete"
  • There's still a todo for the file name, but that's imo not important because of my next point
  • I don't really like the json approach anymore tbh. I think we should go with a more direct cli interaction. What i have in mind right now would be a cli argument that takes an array of sizes, for example tauri icon --extra 123,420,1337, which just reuses the existing png loop, including the default naming scheme. I think letting the users rename the files if needed is not too much to ask for.

@khuongduy354
Copy link
Contributor Author

khuongduy354 commented Dec 8, 2022 via email

cargo tauri icon icon.png --extra 13,15,17\nthis generate png icons: 13x13.png, 15x15.png,17x17.png
@khuongduy354
Copy link
Contributor Author

I made some changes, can you have a look? @FabianLars . It remove json config, and --extra flags generate custom sizes .png with same naming scheme.
Should the --extra generate additional besides default png sizes, or it should ignore default ones?

@FabianLars
Copy link
Member

Should the --extra generate additional besides default png sizes, or it should ignore default ones?

In my opinion it should create additional files, and keep building the default at the same time.

@khuongduy354
Copy link
Contributor Author

khuongduy354 commented Dec 11, 2022

How is it? If you decide to take it, I'll cleanup the comments and test icon files.
@FabianLars

@FabianLars
Copy link
Member

FabianLars commented Dec 12, 2022

Yeah i like that. Another thing we may wanna look into is de-duplication so that we don't end up generating the same icon twice - that can be added after the cleanup ofc.

Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

Thanks so far! Only some small-ish things left. On top of what's mentioned in the comments we also need a changefile, example: https://github.com/tauri-apps/tauri/blob/dev/.changes/is-minimized.md

tooling/cli/src/icon.rs Outdated Show resolved Hide resolved
tooling/cli/src/icon.rs Outdated Show resolved Hide resolved
tooling/cli/src/icon.rs Outdated Show resolved Hide resolved
tooling/cli/src/helpers/config.json Outdated Show resolved Hide resolved
tooling/cli/src/icon.rs Outdated Show resolved Hide resolved
tooling/cli/src/icon.rs Outdated Show resolved Hide resolved
FabianLars
FabianLars previously approved these changes Dec 14, 2022
Copy link
Member

@FabianLars FabianLars left a comment

Choose a reason for hiding this comment

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

imo it's good as-is, thanks!

2 potential changes/improvements we may wanna think about anyway:

  • make the extra_icon_sizes arg an Option so that all the new code is basically skipped for most users
  • If we want to make users able to generate 256 and 512 icons with the default name (on top of the 2 hard coded names), we'd have to hard code the creation of the 128x128@2x.png and icon.png icons outside the for loop.
    At this point we could get rid of the whole dedup logic by also hard coding the removal of 32px and 128px icons from the extra icons vec 🤷

@khuongduy354
Copy link
Contributor Author

I don't really get making extra_icon_sizes as Option.

@khuongduy354
Copy link
Contributor Author

I altered the icon sizes, as you said, it removes the dedup logic, looks better to me. Do u want it or switch back to the old commit ?

@FabianLars
Copy link
Member

I don't really get making extra_icon_sizes as Option.

Doesn't really matter anymore. Without the dedup logic it would add more visual clutter than its worth.

One last thing: The 32 and 128 icon sizes are missing now.

@khuongduy354
Copy link
Contributor Author

So it's done after switching back to prev commit ?

@FabianLars
Copy link
Member

That would introduce the 256 / 512 naming problem from earlier. Honestly just do something like this again

for size in [32, 128].into_iter().chain(extra_icon_sizes) {}

and call it a day. We had so many iterations that i'm not sure anymore what i'd like to see at this point 😅

Thanks for following through btw!

FabianLars
FabianLars previously approved these changes Dec 22, 2022
@FabianLars
Copy link
Member

Sorry that it was such a pain. I'm quite new to this PR reviewing stuff x)

Thank you again!

FabianLars
FabianLars previously approved these changes Dec 22, 2022
@khuongduy354
Copy link
Contributor Author

Thx a bunch, Im a newbie and was busy recently, so i cant focus entirely on this, it such a pain. Anyways, thanks a lot for your guide

@lucasfernog
Copy link
Member

LGTM, but maybe we should change it to only run the user-specified icon sizes? Like instead of running tauri icon --extra-png 24 48 to generate all Tauri icons + the extra ones, we could let the user run tauri icon --png 24 48 --output ./assets/img.

@lucasfernog lucasfernog changed the title feat(cli): add option to make custom icon sizes. feat(cli): add option to make custom icon sizes, closes #5121 Dec 27, 2022
@FabianLars
Copy link
Member

That was indeed the initial idea from the others, but i thought it feels a bit weird (idk why) so i asked to start with this approach first and wait for other opinions

@lucasfernog
Copy link
Member

I've pushed that. We have time to get feedback and revert it if it feels weird :) we still need to audit the next release anyway.

@lucasfernog lucasfernog merged commit 9d21441 into tauri-apps:dev Dec 27, 2022
luoffei pushed a commit to luoffei/tauri that referenced this pull request Dec 29, 2022
… (tauri-apps#5246)

Co-authored-by: Fabian-Lars <fabianlars@fabianlars.de>
Co-authored-by: Lucas Nogueira <lucas@tauri.studio>
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

3 participants