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

Web Support and Refactoring #155

Conversation

personalizedrefrigerator
Copy link

@personalizedrefrigerator personalizedrefrigerator commented Feb 16, 2020

Overview

This pull request adds support for web-based favicons and Progressive Web App launcher icons.

Summary of Changes

  • Added web.dart. This script crops, resizes, and copies icons specified in configuration files.
  • Added tests for web.dart. Additional tests were necessary for the new configuration script.
  • Updated example projects.
  • Refactoring & general bug fixes.

Notes

  • The page's updated favicon is not shown when running flutter run -d chrome or flutter run -d web-server. The project must first be built for web, then served from the build directory (see the README).
  • Several directories that are automatically generated by flutter create . have been removed from the example projects and added to the project's .gitignore file. This minimizes the number of changes committed by PRs that modify the example projects, but does mean users will have to run flutter create . in each of the example project directories (see the README in the default example project).
    • Most of the 264 files GitHub lists as "changed" are deleted files! Most were in the example project's ios and android directories! Please, don't let this large number stop you from reviewing this PR!

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Feb 17, 2020

Supporting multiple icon sets

The android.dart and ios.dart both support sets of different launcher icons (those with path specified instead of a boolean when support for the platform is enabled/disabled). web.dart does not support this.

Ways support might be added

  • Creation of a backup folder to where current icons are copied before replacement of the default icon set. This could permit saving favicon.png in addition to the files in the icons folder.
  • Overwrite favicon.png without saving it, but change the launcher icon folder to the new, requested folder assuming that favicon.png can be generated from the contents of the icons folder.

Suggestions are welcome!

Copy link

@bean5 bean5 left a comment

Choose a reason for hiding this comment

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

This introduced app icon generation for web and also updated the example app. Although I'd like to see a lot of that introduced in a separate ticket, I understand why they were bundled here.

When I run the example, it works. However, when I run it on one of my own icons, android and ios work, but web gives this error:

RangeError (index): Index out of range: index must not be negative: -8
pub finished with exit code 2

@bean5
Copy link

bean5 commented Feb 18, 2020

@personalizedrefrigerator This looks good to me, but I'm not approved to approve and merge. Also, I'm wondering why iOS and Android can handle my icon, but web cannot. Maybe I've formatted my main icon incorrectly?

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Feb 19, 2020

Fixed!

@bean5 Thank you! There was an error in web.dart's cropping code -- it didn't correctly crop images with greater heights than widths. I have fixed the issue and added tests for it.

@bean5
Copy link

bean5 commented Feb 22, 2020

Looks good to me! Web build fine this time!

@bean5
Copy link

bean5 commented Feb 22, 2020

@MarkOSullivan94 was there something you wanted changed before merging this?

…on 0.8.0. Please change this to a more-appliccable version if one exists!
@MarkOSullivan94
Copy link
Collaborator

MarkOSullivan94 commented Feb 23, 2020

I actually haven't got round to testing out Flutter Web yet and feel merging this PR would require someone from the community to step up and keep an eye on any Web related issues / suggestions

Happy to merge in after reviewing this PR if either @bean5 or @personalizedrefrigerator be happy to do this?

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Feb 23, 2020

@MarkOSullivan94 I would be happy to help!

Consider Before Merging

  • Before merging this pull request, it might be worthwhile to consider adding support for custom output directories for web favicons and launcher icons.
    As mentioned before, design suggestions related to this issue are welcome!
  • I assigned version code 0.8.0 to the new release. I originally commented that this version might be inappropriate, given the lack of breaking changes; however, I misstated the version code. I believe 0.8.0 is appropriate -- new functionality was added.

@bean5
Copy link

bean5 commented Feb 25, 2020

@personalizedrefrigerator

7.5.0 would make more sense now that I know that the web cropping issue wasn't indicative of a compatibility issue.

@bean5
Copy link

bean5 commented Feb 25, 2020

@MarkOSullivan94 whenever I start make a new project, I'm going to be generating an icon for web, so testing will happen naturally for web.

While for most projects I believe that a web branch/fork should exist until flutter supports web in their stable channel, I don't think that is necessary for this project because it is a dev dependency. I don't see any harm in merging this once the version number is revised to reflect that it is backwards-compatible (ie 7.*).

@bean5
Copy link

bean5 commented Feb 25, 2020

@MarkOSullivan94 feel free to add me as a contributor if it helps. I can't commit to an official amount of time, but adding me shouldn't hurt anything. :)

@personalizedrefrigerator
Copy link
Author

Whoops! My fault! The version code was 0.8.0 and 0.7.5, not 8.0.0 and 7.5.0! I support the switch to 0.8.0 but understand any wish to change this.

@bean5
Copy link

bean5 commented Feb 26, 2020

Oh, 0.8.0 is fine. It shows backwards-compatibility.

LGTM

@bean5
Copy link

bean5 commented Mar 5, 2020

Until this gets merged in, I'm relegated to using the fork instead. Just sayin'.

@TheJulianJES
Copy link

Love it, but I did get the following error when I tried it:

Overwriting web favicon and launcher icons...
FileSystemException: Cannot open file, path = 'web/icons/Icon-192.png' (OS Error: No such file or directory, errno = 2)

Creating the icons folder in the web directory fixed it though!

@cocacrave
Copy link

Does the web icon come out really pixelated for anyone else?
Comparing same size for android/ios vs web generated icons, they come out pixelated.
I'm using Flutter beta v1.15.17 on MacOS 10.15.3.

dev_dependencies:
  flutter_test:
    sdk: flutter
  flutter_launcher_icons: ^0.7.4

dependency_overrides:
  flutter_launcher_icons:
    git:
      url: https://github.com/personalizedrefrigerator/flutter_launcher_icons
      ref: master

Am I doing something wrong?

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Mar 25, 2020

Thanks! I'm looking into it! Are you referring to the favicon or all generated launcher icons? What are the dimensions of the icon you are attempting to distribute?

Copy link
Collaborator

@MarkOSullivan94 MarkOSullivan94 left a comment

Choose a reason for hiding this comment

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

Left a couple of comments for you to check out!

Got a feeling it wont be long until this is merged 🙌

example/web/lib/main.dart Outdated Show resolved Hide resolved
example/web/pubspec.yaml Outdated Show resolved Hide resolved
lib/constants.dart Outdated Show resolved Hide resolved
Comment on lines +113 to +122
{bool verbose, String cwd}) {
verbose ??= false;
final String configFile = argResults[fileOption];
cwd ??= './';

String configFile = argResults[fileOption];
final String fileOptionResult = argResults[fileOption];

if (configFile != null) {
configFile = path.join(cwd, configFile);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch!

It'd be worthwhile putting a brief explanation above the declaration of the loadConfigFileFromArgResults so others contributing will be made aware of why cwd parameter was added

lib/main.dart Show resolved Hide resolved
@@ -14,4 +14,4 @@ environment:
sdk: '>=2.3.0 <3.0.0'

dev_dependencies:
test: ^1.11.1
test: '^1.11.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep this is definitely a good idea! Will add in the future 👍

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Nov 23, 2020

The new example projects are called default_example (flutter create . didn't work when called default for this one) and flavors_renameme

Wait, why have you needed to recreate the examples which were there before this PR?

GitHub's diff was showing every deleted file I added copies of the old example projects to limit the size of the diff GitHub displays.

@MarkOSullivan94
Copy link
Collaborator

GitHub's diff was showing every deleted file — I added copies of the old example projects to limit the size of the diff GitHub displays.

Size of the diff is fine don't worry 😃

lib/web.dart Outdated Show resolved Hide resolved
@MarkOSullivan94
Copy link
Collaborator

Last change I'd recommend: Probably best to go ahead and execute the flutter create . command in the web example

This would mean one less step to run the web example 🙂

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented Dec 1, 2020

Done!

78 commits is a lot. Would you like me to try to clean up the commit history (i.e. using git reflog -i or similar)?

@personalizedrefrigerator
Copy link
Author

At present, unlike the Android and iOS launcher icon generators, web.dart cannot be given an alternate icon path (e.g. web: favicon_test.png). I think this is better left as the topic of a followup PR.

Would you like me to try to add this functionality?

@MarkOSullivan94
Copy link
Collaborator

Done!

78 commits is a lot. Would you like me to try to clean up the commit history (i.e. using git reflog -i or similar)?

Yep that would be appreciated!

At present, unlike the Android and iOS launcher icon generators, web.dart cannot be given an alternate icon path (e.g.
web: favicon_test.png). I think this is better left as the topic of a followup PR.

Would you like me to try to add this functionality?

I agree this is better to leave for a follow up PR

@MarkOSullivan94
Copy link
Collaborator

@personalizedrefrigerator heads up your commits removes some important files for the default example - guessing you done this before adjusting the .gitignore?

@personalizedrefrigerator
Copy link
Author

@personalizedrefrigerator heads up your commits removes some important files for the default example - guessing you done this before adjusting the .gitignore?

I think the default example only needs pubspec.yaml, assets/images/ and README.md -- everything else is created by flutter create ..

@MarkOSullivan94
Copy link
Collaborator

everything else is created by flutter create ..

Less steps needed for anyone to run the examples the better 🙂

@bean5
Copy link

bean5 commented Mar 4, 2021

Flutter 2.0 was announced today. It officially supports web, Linux, and other platforms. At pub.dev this project promises to be compatible for web at but I don't think it is--unless this is a duplicate branch and it was already done.

I just checked. The default is not building for web.

How can I help get this done? What is blocking this from being finished up?

@leithhobson
Copy link

leithhobson commented Apr 7, 2021

Flutter 2.0 was announced today. It officially supports web, Linux, and other platforms. At pub.dev this project promises to be compatible for web at but I don't think it is--unless this is a duplicate branch and it was already done.

I just checked. The default is not building for web.

How can I help get this done? What is blocking this from being finished up?

The package is stated as compatible as your web project will not be broken by it (in my understanding) however, it does not generate the web favicons.
Yes, it is certainly time that the web support feature is implemented.

@Jwiggiff
Copy link

Jwiggiff commented May 7, 2021

Is there any update/progress on this? Is there anything I could do to help? Would really love some web support :)

@personalizedrefrigerator
Copy link
Author

personalizedrefrigerator commented May 7, 2021

I haven't been using Flutter recently! I'll try to resolve conflicts for this PR (but might have to close it & open another -- resolving conflicts will probably involve a rebase and users of the fork may be having Flutter fetch a specific commit).

Edit: Sorry! I've been busier than expected recently. I hope to find time to do this over the weekend!

@alcemirsantos
Copy link

Just got here coming from Issue #126. I think it would be interesting to link the issue to this PR since they are definitely related and there are other user asking about this there.

@personalizedrefrigerator
Copy link
Author

Closing in favor of #189

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

9 participants