Suggest to Remove Favicon

[ab-tools]

Hello tobozo,

personally, I don't think it's a good idea that you add a favicon in your project.
I mean, the favicon is defined for the whole web server, not only the NTP part your plug-in is responsible for.

As I want to provide an own favicon, I need to see if I can "override" your /favicon.ico route again.
But I do think providing a global server favicon should not be the business of a NTP server add-on in the first place.

Thanks for considering
Andreas

[tobozo]

replying with a favicon smaller than 64 bytes was faster than emitting a 404 error when this project was created, some browser tend to make extra queries when a favicon isn't found and this was affecting stability, sometimes even triggering crashes, so it's more of a safeguard

this is untested but I believe the default favicon can be overriden by attaching your favicon callback right after the WiFiManagerTz init:

    WiFiManagerNS::init( &wifiManager );
    wifiManager.server->on("/favicon.ico", yourFaviconCallback);

[ab-tools]

Thanks for your quick reply again, tobozo.

replying with a favicon smaller than 64 bytes was faster than emitting a 404 error when this project was created, some browser tend to make extra queries when a favicon isn't found and this was affecting stability, sometimes even triggering crashes, so it's more of a safeguard

Having a (optinal) default favicon might make sense to add to the WiFiManager itself then, but it feels at the wrong place in a NTP add-on.

this is untested but I believe the default favicon can be overriden by attaching your favicon callback right after the WiFiManagerTz init:

    WiFiManagerNS::init( &wifiManager );
    wifiManager.server->on("/favicon.ico", yourFaviconCallback);

Tried that, but this crashes the app immediately - seems there is no way to override an existent server route.

I would therefore appreciate if you could make the favicon at least optional, ideally with a compiler directive, so also your icon file data does not get included in code when not used.

Thanks
Andreas

[tobozo]

I made the disabling of the favicon optional since it's there to prevent crashing on ESP8266, however adding a conditional macro just to save 57 bytes on the flash isn't worth the added complexity and documentation overhead it would create.

branch 1.3.3 supports the following syntax to disable the builtin favicon:

WiFiManagerNS::init( &wifiManager, nullptr );

[ab-tools]

Thanks, tobozo, but I found out the issue is a bit "broader" as it looks like that you can only have a single bindServerCallback, I'm unable to define any further routes currently anyway...

I'm just added an additional callback now that is called before you add your routes.
This should solve the problem as well.

Testing right now, if it works as expected, I'll create a pull request for you to check.

Thanks again for your attention
Andreas

[tobozo]

if you make a PR please make sure it's based on branch 1.3.3, there are some significant changes compared to the master

[ab-tools]

Just created the PR for master, but no problem, will rebase to 1.3.3 now.

[ab-tools]

Honestly, it's a bit complicated now as you made all kinds of changes in just a single commit now...

Trying to separate things and create a pull request accordingly.

[ab-tools]

Here you go:
#8

This way the consumer of your library is fully flexible and can add/override any server routes as needed.

The one defining a specific server route first "wins".
This way no need to change anything at your "favicon" route as this can just be overridden now by the new "pre-callback".

[tobozo]

you made all kinds of changes in just a single commit now...

sorry about that, esp8266 support was half way last time I worked on this project so it came with all the changes, and I'm still pushing fixes :-)

thanks again for your PR 👍