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

Preview support via Sailfish.WebView #8

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

roundedrectangle
Copy link
Contributor

@roundedrectangle roundedrectangle commented Nov 21, 2024

The PR is done and ready to be merged. The main change for the user is in the function. It now has a new argument previewType which can be set from enumeration LinkPreviewType. It accepts:

  • auto-detection (checks for Sailjail permissions, internet availability and WebView module installation)
  • internet-only detection (to save some performance from checking if WebView components are installed)
  • force disable
  • force enable

Internally, it also loads the webview only once the attached page with it is opened, destroys it when the page is closed

However there are still some things which could be improved (they aren't that important):

  • LinkPreviewType is not a real enumeration
  • The WebView itself could be exposed to the user
  • Besides the dot at the top, you can't know if preview is available
  • Code in private folder isn't as clean as it could be ideally

@ichthyosaurus
Copy link
Member

Thank you! It looks very good on a quick glance but I haven't tested it yet.

LinkPreviewType is not a real enumeration

That's ok. A real enum would need C++ code and I'd prefer to keep this module as light weight as possible.

The WebView itself could be exposed to the user

Maybe but not really necessary, I think. It can be implemented later if the need arises.

Besides the dot at the top, you can't know if preview is available

I think adding a note at the bottom of the page would be a good idea. I don't want apps to do any network access without explicit user interaction, and accidentally swiping could load a link that the user didn't want to load.

Some more nitpicks:

  • does your method distinguish between wifi and mobile network?
  • please add a "Permissions" section to the readme like here
  • please revert the lupdate commit because that often causes merge conflicts

Again, thank you for your effort! It's a really nice addition to the module! :)

@roundedrectangle
Copy link
Contributor Author

Hello!

does your method distinguish between wifi and mobile network?

Didn't really understand the question. I believe this method can check the network type. But in the current implementation it only checks if SailfishOS itself thinks if the device is online, and doesn't check what the network type is. I think SailfishOS UI (lipstick) itself uses same method as here.

please add a "Permissions" section to the readme like here

please revert the lupdate commit because that often causes merge conflicts

Okay, I'll do that.

I think adding a note at the bottom of the page would be a good idea. I don't want apps to do any network access without explicit user interaction, and accidentally swiping could load a link that the user didn't want to load.

I'll think about that and add a note like you suggested or something else.

Once I'll finish everything you suggested, I'll comment here.

README.md Outdated
@@ -34,6 +34,19 @@ Label {
}
```

## Permissions

Some permissions are required for WebView-based preview support in this module to work in Sailjail. This only affects apps intended for the Harbour store that show local video files.
Copy link
Member

Choose a reason for hiding this comment

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

"that show local video files" copy-paste oops ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, missed that!

@roundedrectangle
Copy link
Contributor Author

I finished everything you suggested. But I just found another issue with this: it shows previews for URLs like phone numbers which is pointless because preview does the same thing as the button. Please wait for me to add a fix for this before merging the PR.

@roundedrectangle
Copy link
Contributor Author

roundedrectangle commented Nov 27, 2024

Hello again, I fixed that issue and also added an animation for the preview indication label.

The PR is ready to be merged, everything is fixed.

@ichthyosaurus
Copy link
Member

Thank you @roundedrectangle, nice job! I'll be traveling the next few days but I will review it once I'm back!

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.

2 participants