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

feat: external sticker cache #29

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

ruattd
Copy link
Contributor

@ruattd ruattd commented Jan 24, 2024

实验性功能: 在外部存储缓存贴纸以方便与其他应用共享 (如 QQ 等)

我不确定它会不会炸因为我没法编译 Nagram

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: The pull request introduces an experimental feature that allows external storage caching of stickers for sharing with other applications, such as QQ. It includes changes to the MediaDataController to handle the caching logic and adds a new ConfigCellButton class to manage button cells in the settings activity. Additionally, it modifies the NekoExperimentalSettingsActivity to integrate the new button and its associated actions.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Review the lifecycle management of the new UI components to ensure they are properly cleaned up to prevent memory leaks.
  • Consider the appropriate timing and lifecycle events for refreshing the external sticker storage state to ensure it is updated accurately.
  • Ensure that the new feature's error handling is robust, especially in the caching process, to prevent application crashes.
  • Check for null references in callback methods to avoid null pointer exceptions.
  • The PR description could be improved by providing more context on the feature's impact and confirming that it has been tested, as the current description expresses uncertainty about the feature's stability.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -133,6 +152,8 @@ public void onItemClick(int id) {
}
});

refreshExternalStickerStorageState(); // Cell (externalStickerCacheRow): Refresh state
Copy link

Choose a reason for hiding this comment

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

suggestion (llm): Consider moving the call to refreshExternalStickerStorageState to a more appropriate lifecycle method if onFragmentCreate is not the best place to refresh the state, ensuring that the state is refreshed at the correct time.

@ruattd ruattd marked this pull request as draft January 24, 2024 15:28
@NextAlone
Copy link
Owner

@ruattd 加一个保存到相册不就行了

@NextAlone
Copy link
Owner

NextAlone commented Jan 24, 2024

Screenshot_2024-01-25-00-25-47-031_xyz.nextalone.nnngram.jpg

@ruattd
Copy link
Contributor Author

ruattd commented Jan 24, 2024

只是我太懒了想让它自动完成这个过程(外加隔壁 qa 有人写的阴间代码不从相册里面读

@ruattd ruattd marked this pull request as ready for review January 30, 2024 07:54
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: The pull request introduces a new experimental feature that allows caching stickers in external storage for sharing with other applications. It includes changes to various classes to support this functionality, such as adding new menu options for managing the external sticker cache and handling the selection of external directories for the cache.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Review the synchronous calls to cacheStickers to ensure they do not adversely affect the main thread's performance. Consider using asynchronous patterns if necessary.
  • Add error handling for cases where the ACTION_OPEN_DOCUMENT_TREE intent cannot be resolved by any app on the device to prevent application crashes.
  • Clarify the usage of String() method on getExternalStickerCache() to ensure it is the correct way to check if the external sticker cache configuration is not blank.
  • Ensure that the new feature is well-documented within the codebase to help future maintainers understand its purpose and implementation.
  • Verify that all new user-facing features, such as menu options for managing the external sticker cache, are intuitive and provide a good user experience.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

NaConfig.INSTANCE.getExternalStickerCache().setConfigString("");
} else {
Intent intent = new Intent(Intent.ACTION_OPEN_DOCUMENT_TREE);
startActivityForResult(intent, INTENT_PICK_EXTERNAL_STICKER_DIRECTORY);
Copy link

Choose a reason for hiding this comment

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

suggestion (llm): It's good practice to handle the potential ActivityNotFoundException that might be thrown if no app can handle the ACTION_OPEN_DOCUMENT_TREE intent.

@@ -1952,6 +1953,9 @@ public void addNewStickerSet(TLRPC.TL_messages_stickerSet set) {
loadHash[type] = calcStickersHash(stickerSets[type]);
getNotificationCenter().postNotificationName(NotificationCenter.stickersDidLoad, type, true);
loadStickers(type, false, true);

// Na: [ExternalStickerCache] cache sticker sets
ExternalStickerCacheHelper.cacheStickers();
Copy link

Choose a reason for hiding this comment

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

suggestion (llm): Consider the impact of invoking cacheStickers method synchronously here. If this operation is I/O intensive or time-consuming, it might be beneficial to run it in a separate thread or use an asynchronous approach to avoid blocking the main thread.

@@ -865,6 +869,10 @@ public void requestLayout() {
optionsButton.addSubItem(2, R.drawable.msg_link, LocaleController.getString("CopyLink", R.string.CopyLink));
optionsButton.addSubItem(3, R.drawable.msg_qrcode, LocaleController.getString("ShareQRCode", R.string.ShareQRCode));
optionsButton.addSubItem(menu_archive, R.drawable.msg_archive, LocaleController.getString("Archive", R.string.Archive));
if (!NaConfig.INSTANCE.getExternalStickerCache().String().isBlank()) {
Copy link

Choose a reason for hiding this comment

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

question (llm): The method call String() on getExternalStickerCache() seems unusual. Verify that String() is the correct method to call for checking if the external sticker cache configuration is not blank.

@ruattd
Copy link
Contributor Author

ruattd commented Jan 30, 2024

应该没有什么问题了(至少我没测出什么问题来)

@omg-xtao omg-xtao added the enhancement New feature or request label Jan 30, 2024
@omg-xtao omg-xtao self-requested a review January 30, 2024 08:30
@ruattd ruattd force-pushed the feat-external-sticker-cache branch from b3af2ab to a102cd6 Compare January 30, 2024 09:31
@NextAlone
Copy link
Owner

重新打开以触发 pr check

@NextAlone NextAlone closed this Jan 30, 2024
@NextAlone NextAlone reopened this Jan 30, 2024
@omg-xtao omg-xtao merged commit 97c9309 into NextAlone:dev Jan 30, 2024
omg-xtao referenced this pull request in TeamPGM/Nagram Jan 30, 2024
@ruattd ruattd deleted the feat-external-sticker-cache branch January 30, 2024 11:44
LiuYi0526 pushed a commit to LiuYi0526/Nagram that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants