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

pwa support for elastic skin #9354

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

pwa support for elastic skin #9354

wants to merge 4 commits into from

Conversation

the0ne
Copy link

@the0ne the0ne commented Feb 14, 2024

As progressive web app support is great nowadays on all major operating system platforms, roundcube can be used even more convenient if it supports this standard. Roundcube can look like it was installed as a 'real' app, have a proper launcher icon, etc.
Even notifications would be possible if the service worker inside sw.js is further enhanced in the future.

This PR should solve #6510.

@@ -39,6 +39,47 @@
} catch (e) { }
</script>
<roundcube:endif />
<roundcube:if condition="env:action != 'print' && !config:devel_mode" />
<link rel="manifest" href="/manifest.json">
Copy link
Contributor

Choose a reason for hiding this comment

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

one high-res icon should be enough, also it is much easier to replace it with a custom icon then

Copy link
Author

Choose a reason for hiding this comment

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

You'd have to discuss this point with the guys that defined PWA I guess 😊

Copy link
Member

Choose a reason for hiding this comment

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

https://developer.apple.com/design/human-interface-guidelines/app-icons#App-icon-sizes says:

You can let the system automatically scale down your 1024x1024 px app icon to produce all other sizes

For MS Windows and Android, at least 512x512 pixels are required. So to me, too, this sounds like way fewer icons are possible to make it work.

I guess for optimal icon displaying more sizes are better, but for this PR to be mergeable we need to find a solution that keeps changing the icon feasible. (Nothing stops anyone from adding more icons at all.)

@the0ne Would you be willing to change this to reach a possible compromise?

Copy link
Author

Choose a reason for hiding this comment

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

I'd be really happy if somebody would step in and take care of those changes.
In the next several months (or even longer) I won't be able to take care about this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the feedback!

@alecpl
Copy link
Member

alecpl commented Feb 25, 2024

I don't think I'm gonna accept this. Maybe it should/could become a plugin.

People really need a way to replace the icon easily. So, maybe we could generate all icon sizes from a single icon when they are requested.

Also things like "application-name": "HS Webmail", can't be there.

@the0ne
Copy link
Author

the0ne commented Feb 25, 2024

Yeah, the application name clearly wasn't meant like this.
I've updated it to Roundcube Webmail but obviously you can put there whatever you like.
Regarding the images, is there some mechanism in RC that could be used for this?

@alecpl
Copy link
Member

alecpl commented Feb 25, 2024

rcube_image::resize()

@the0ne
Copy link
Author

the0ne commented Feb 25, 2024

rcube_image::resize()

😂👍🏻

I don't think that's gonna cut it... 😉

Anyway, I've made this PR as a proposal on how this whole feature can easily be implemented.

I don't feel you're particularly interested anyway and I don't intend to play a game where you request random changes and expect me to search for solutions in your software.

So just let me know and I'll happily cancel the PR if that's your wish. Thanks! 🙂👌🏻

@miaulalala
Copy link
Contributor

miaulalala commented May 6, 2024

Will need something like #7709

I think this is a great idea, would love to see it, but the current login flow won't make this an easy thing as you'll be logged out any time the window closes.

@the0ne
Copy link
Author

the0ne commented May 6, 2024

Wasn't an issue on the installation I host because anyway I got the 'persistent_login' plugin installed which takes care of this automatically.


self.addEventListener( "install", function ( event ) {

console.log( "Installing the service worker!" );
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the logging from this file (or make it helpful beyond debugging).

Copy link
Member

Choose a reason for hiding this comment

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

Actually this sets up caching network data, but that data doesn't ever get used, right? So please either complete that feature or remove it.

@pabzm
Copy link
Member

pabzm commented May 29, 2024

FTR: #9014 is "prior art", but much less elaborate.


} );

self.addEventListener( "fetch", function ( event ) {

Choose a reason for hiding this comment

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

Roundcube doesn't use fetch in most of its code (it's older code that uses XMLHttpRequest), so this won't actually do anything.

@Daniel15
Copy link

IMO the service worker should be removed from this PR and done in a separate one. Offline access is a major feature of PWAs, but it'd be non-trivial to implement.

Adding the metadata and icon would be beneficial. Maybe this could be split into three PRs:

  1. Add the metadata
  2. Add the icon
  3. Implement offline support using the service worker

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.

6 participants