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

Highlights share #9153

Merged
merged 10 commits into from Jun 3, 2022
Merged

Highlights share #9153

merged 10 commits into from Jun 3, 2022

Conversation

uroybd
Copy link
Contributor

@uroybd uroybd commented Jun 1, 2022

This PR enables users to share exported highlights on Android.


This change is Reviewable

@pazos
Copy link
Member

pazos commented Jun 1, 2022

@uroybd: made a few changes.

Mostly:

  • don't mix logic. Use canShareText as a precondition for doShareText. Avoid specific android checks.
  • disabled share submenu if there's no document opened (like the export one).

@uroybd
Copy link
Contributor Author

uroybd commented Jun 1, 2022

@uroybd: made a few changes.

Mostly:

  • don't mix logic. Use canShareText as a precondition for doShareText. Avoid specific android checks.
  • disabled share submenu if there's no document opened (like the export one).

It looks much cleaner now. I've also made the changes you've asked.

Should I add the same functionality for text, JSON, and html?

@uroybd
Copy link
Contributor Author

uroybd commented Jun 1, 2022

@pazos

Also, I need some help from luajit part.

The doShareText function can do with two modifications:

  1. Optional EXTRA_TITLE field. Some apps might use it to set the note/file name. We can send a generated title using this.

  2. Option to set mimetype, defaults to text/plain.

@pazos
Copy link
Member

pazos commented Jun 1, 2022

@pazos

Also, I need some help from luajit part.

The doShareText function can do with two modifications:

  1. Optional EXTRA_TITLE field. Some apps might use it to set the note/file name. We can send a generated title using this.
  2. Option to set mimetype, defaults to text/plain.

Both are doable but not for this PR.
They will require some familiary with https://github.com/koreader/android-luajit-launcher.

Also the signature of share text is different https://github.com/koreader/android-luajit-launcher/blob/master/app/src/main/java/org/koreader/launcher/extensions/ActivityExtensions.kt#L258, because it was designed to deliver text to specific packages using specific actions, so something like that requires a reestructure I'm not willing to do in short run.

@pazos
Copy link
Member

pazos commented Jun 1, 2022

Should I add the same functionality for text, JSON, and html?

Sure, go ahead.

Also you can test them on the emulator with

diff --git a/frontend/device/sdl/device.lua b/frontend/device/sdl/device.lua
index cef65e08..8b86498a 100644
--- a/frontend/device/sdl/device.lua
+++ b/frontend/device/sdl/device.lua
@@ -122,6 +122,10 @@ local Emulator = Device:new{
     canPowerOff = yes,
     canReboot = yes,
     canSuspend = yes,
+    canShareText = yes,
+    doShareText = function(self, text)
+        logger.info("sharing text", text)
+    end,
 }

@uroybd
Copy link
Contributor Author

uroybd commented Jun 2, 2022

@pazos

Instead of isDocReady I'm using isUIReady which doesn't check for enabled formats. Enabling formats are required for file generation. However, for one-off sharing by directly selecting a format it is not necessary.

@uroybd uroybd changed the title WIP: Highlights share Highlights share Jun 2, 2022
@uroybd
Copy link
Contributor Author

uroybd commented Jun 2, 2022

Removed WIP.

@uroybd uroybd requested a review from pazos June 2, 2022 17:23
@pazos
Copy link
Member

pazos commented Jun 2, 2022

@pazos

Instead of isDocReady I'm using isUIReady which doesn't check for enabled formats. Enabling formats are required for file generation. However, for one-off sharing by directly selecting a format it is not necessary.

Agree on the logic as it is now, not on the names. The new isUIReady is indeed isDocReady and should be called that way. The other thing, until now called isDocReady, can be renamed isReadyToExport or something similar (honoring the name that it used to have before the refactor and giving a proper name of what it does).

Copy link
Member

@pazos pazos left a comment

Choose a reason for hiding this comment

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

Except for renaming a couple of methods and removing the timestamp the rest looks quite good and it is almost ready for prime time :)

👍

@uroybd
Copy link
Contributor Author

uroybd commented Jun 3, 2022

Except for renaming a couple of methods and removing the timestamp the rest looks quite good and it is almost ready for prime time :)

+1

Renaming and other changes based on the review are done. :)

@pazos pazos added this to the 2022.06 milestone Jun 3, 2022
Copy link
Member

@pazos pazos left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants