Skip to content

Conversation

@christophehenry
Copy link
Contributor

@christophehenry christophehenry commented Jan 27, 2025

Implement #251

Screenshot_20250127_132903
Screenshot_20250127_132928

@christophehenry christophehenry force-pushed the custom-share-intent-text branch from 48349d0 to d32c683 Compare January 27, 2025 12:35
@christophehenry christophehenry changed the title Implement #251: customize shared text using template setting Customize shared text using template setting Jan 27, 2025
@Shinokuni
Copy link
Member

Hello, thank you for this! I'm sorry for not answering your issue before you started working on this. So let's see.

I got a bit of a hard time making the template to work, due to a few things:

  • I took the textfield placeholder in CustomShareIntentTextWidget as the proper template and thus I didn't understand why it wouldn't work when I saved it and clicked on the share buttons. I think the placeholder should be real text, firstly to avoid confusion from users and secondly to let them work and modify a already working template
  • When you launch the app with the template parameter disabled, if you enable it and go back to the timeline tab, the template won't work because you get it only once at launch with customShareIntentTpl = preferences.customShareIntentTpl.flow.first(). You won't be able to react to its changes. This should go in getTimelinePreferences

Otherwise it works greatly, I would have a few other things to say:

  • When sharing with the template activated for the first time, it takes a significant amount of time before the android share dialog shows up. Is it due to the PebbleEngine engine building? Are you able to reproduce this?
  • When sharing from the timeline list, the author variable isn't available. We need it to get it from the database
  • Could you add a french translation? Otherwise I can handle it myself

@christophehenry
Copy link
Contributor Author

christophehenry commented Jan 28, 2025

Is it due to the PebbleEngine engine building? Are you able to reproduce this?

I believe so because it seems to be doing a lot of introspection and since it's static, it seems to happen only the first time. I don't know what to do about it. Maybe a Koin injection? I'm not sure whether Koin modules are lazy or not.

Edit: So I just tested and, yes, they're lazy. Except for importing ShareIntentTextRenderer in ReadropsApp to force instanciation of the static variable, I don't know what to do.

Could you add a french translation? Otherwise I can handle it myself

Yes, I will do. Please remind me to do so if I forget.

I will try to fix your other comments tomorrow.

@christophehenry christophehenry force-pushed the custom-share-intent-text branch from d32c683 to 00bc58e Compare January 29, 2025 11:58
@christophehenry
Copy link
Contributor Author

christophehenry commented Jan 29, 2025

I think I fixed everything except for the latency due to the PebbleEngine initialization.

I think the placeholder should be real text

I added a button to fill the input with a default template. I updated the images in the description of the PR according to that.

@christophehenry
Copy link
Contributor Author

I fixed the R8 compilation error with the last commit.

When sharing with the template activated for the first time, it takes a significant amount of time before the android share dialog shows up.

This does not seem to reproduce with the beta and release builds so I guess it's fine. The compiler must be optimizing the code.

@codecov
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 8.33333% with 176 lines in your changes missing coverage. Please review.

Project coverage is 19.77%. Comparing base (15eb463) to head (026eb2b).

Files with missing lines Patch % Lines
...ferences/components/CustomShareIntentTextWidget.kt 0.00% 86 Missing ⚠️
...a/com/readrops/app/util/ShareIntentTextRenderer.kt 18.75% 26 Missing ⚠️
...ops/app/more/preferences/PreferencesScreenModel.kt 0.00% 18 Missing ⚠️
app/src/main/java/com/readrops/app/util/Utils.kt 0.00% 12 Missing ⚠️
...readrops/app/more/preferences/PreferencesScreen.kt 0.00% 11 Missing ⚠️
...main/java/com/readrops/app/item/ItemScreenModel.kt 0.00% 8 Missing ⚠️
...e/preferences/components/SwitchPreferenceWidget.kt 0.00% 8 Missing ⚠️
...a/com/readrops/app/timelime/TimelineScreenModel.kt 0.00% 7 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #254      +/-   ##
=============================================
- Coverage      19.94%   19.77%   -0.17%     
  Complexity       448      448              
=============================================
  Files            188      190       +2     
  Lines           9838     9998     +160     
  Branches        1545     1563      +18     
=============================================
+ Hits            1962     1977      +15     
- Misses          7760     7905     +145     
  Partials         116      116              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christophehenry
Copy link
Contributor Author

Uh. You want me to add integration tests? Can do but I really hate writing android tests.



class PreferencesScreen : AndroidScreen() {
class PreferencesScreen : AndroidScreen(), KoinComponent {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need KoinComponent anymore, don't we?

@christophehenry christophehenry force-pushed the custom-share-intent-text branch 4 times, most recently from 99487a3 to eebb46f Compare February 4, 2025 16:39
@christophehenry christophehenry force-pushed the custom-share-intent-text branch from eebb46f to 026eb2b Compare February 4, 2025 16:40
@christophehenry
Copy link
Contributor Author

Alright, the last commit applies your suggestions.

@christophehenry christophehenry force-pushed the custom-share-intent-text branch from 97a2ed6 to 026eb2b Compare February 6, 2025 10:26
@Shinokuni Shinokuni merged commit 9e7e943 into readrops:develop Feb 12, 2025
2 checks passed
@Shinokuni
Copy link
Member

Thank you for this work! I hope to be able to release a new version soon with this, most of the work has been done.

@christophehenry
Copy link
Contributor Author

Awesome! Thank you!

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