Skip to content

refactor: replace dynamic string composition with explicit string resources#845

Merged
swakwork merged 1 commit into
crimera:devfrom
krvstek:refactor
May 27, 2026
Merged

refactor: replace dynamic string composition with explicit string resources#845
swakwork merged 1 commit into
crimera:devfrom
krvstek:refactor

Conversation

@krvstek

@krvstek krvstek commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

This PR refactors SettingsAboutFragment by replacing the strEnableRes and strRemoveRes methods with direct strRes calls.

Previously, the code dynamically composed strings (e.g., adding Enable... or Remove... prefixes to feature names). This approach caused significant localization issues due to varying grammar rules, genders, and word orders across different languages. By using explicit string resources, the translation process becomes much easier and the UI will look cleaner across all supported languages.

@krvstek krvstek force-pushed the refactor branch 2 times, most recently from 67dbc9d to 8dae7d0 Compare March 20, 2026 13:48
@krvstek krvstek marked this pull request as ready for review March 20, 2026 13:50
@kitadai31

Copy link
Copy Markdown
Contributor

Have you checked "Delete entries from database" screen?

@krvstek

krvstek commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Have you checked "Delete entries from database" screen?

I didn't touch the database string or the array itself, so it shouldn't be affected. I actually can't build and check it locally on my end right now though, could you verify if it looks fine on your side?

@kitadai31

kitadai31 commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

As I expected, the new strings have an incorrect meaning in the "Delete entries from database" dialog.

deletefromdb

It used to look like this:

old_strings

Therefore, it may be necessary to keep the dynamic string compositions for the items included in the "delete entries from database" dialog.
Other changes are fine.

@krvstek

krvstek commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

As I expected, the new strings have an incorrect meaning in the "Delete entries from database" dialog.

deletefromdb It used to look like this: old_strings Therefore, it may be necessary to keep the dynamic string compositions for the items included in the "delete entries from database" dialog. Other changes are fine.

If I knew how to compile this properly (I've read the dev docs but honestly got a bit lost there, though I'll try to figure it out and test it myself when I have some time), this issue probably wouldn't have happened.

However, I'm thinking about adding dedicated strings specifically for this dialog. I know it might lead to some duplication, but IMO it's the simplest approach and stays true to my goal of making translations easier to handle.

Moving this PR to draft until I come up with a fix in my spare time.

@krvstek krvstek marked this pull request as draft March 29, 2026 14:24
@kitadai31

kitadai31 commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

However, I'm thinking about adding dedicated strings specifically for this dialog. I know it might lead to some duplication, but IMO it's the simplest approach and stays true to my goal of making translations easier to handle.

I completely agree.

After reconsidering, I realized that since there are only nine strings, it wouldn't be a burden to make it dedicated strings.

I also realized that dynamically compositing these strings results in a grammatical problem.
In this dialog, any items such as "Promoted posts" should start with a capital letter, but if we do that, the "P" in "Hide Promoted posts" will be incorrectly capitalized.

So as you said, it's better to add a dedicated string for the "delete entries from database" items.

@krvstek krvstek marked this pull request as ready for review March 29, 2026 21:09
@krvstek

krvstek commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

fixed.

Screenshot_20260329-230608 Screenshot_20260329-230612

@kitadai31

Copy link
Copy Markdown
Contributor

In arrays.xml, with the addition of piko_array_db_del_items, piko_array_ads_hooks is no longer used.


Also, is it possible to include fixes of some typo in string key in this pull request?

piko_pref_hide_detailed_posts -> piko_pref_hide_related_posts
piko_pref_db_detailed_posts -> piko_pref_db_related_posts

This originates from an old typo.
"Tweet Detail Related Tweets" was mistakenly abbreviated to "Detailed".
I fixed the text at 3cf5f9e, but I didn't fix the string key at that time.

Since changing string keys later can be a bit cumbersome, I think it's best to do it in conjunction with this major refactoring.
This also helps the translator understand the text.

@krvstek

krvstek commented Apr 4, 2026

Copy link
Copy Markdown
Contributor Author

In arrays.xml, with the addition of piko_array_db_del_items, piko_array_ads_hooks is no longer used.

Also, is it possible to include fixes of some typo in string key in this pull request?

piko_pref_hide_detailed_posts -> piko_pref_hide_related_posts piko_pref_db_detailed_posts -> piko_pref_db_related_posts

Done.


@crimera @swakwork Two quick questions while I'm at it:

  • I noticed piko_title_piko_integrations in the strings. It's referenced in SettingsAboutFragment.java but doesn't actually show up anywhere in the UI. Is it safe to just remove it? (I'm guessing it's a leftover from RxVxnced).
  • Since my refactor already gets rid of strEnableRes and strRemoveRes, we could take it a step further and drop strRes entirely, replacing it with direct use of StringRef. What do you guys think? Is it worth doing it in this PR, or should I leave strRes alone?

@kitadai31

Copy link
Copy Markdown
Contributor

My new commit was merged today, but it relies on an old dynamic string.

Please change this to piko_pref_import_success.
Sorry to keep asking for your help, but thank you in advance.


I noticed piko_title_piko_integrations in the strings. It's referenced in SettingsAboutFragment.java but doesn't actually show up anywhere in the UI. Is it safe to just remove it? (I'm guessing it's a leftover from RxVxnced).

SettingsAboutFragment.java is not from Re*anced, it's genuinely from piko settings screen.
This is a remnant of the process that copies the version number when you pressed the Integration version.
The button has been removed, but it seems they forgot to remove the internal key (preference key) to distinguish buttons.

-        if ( (key.equals(strRes("piko_pref_app_version"))) || (key.equals(strRes("piko_title_piko_patches"))) || (key.equals(strRes("piko_title_piko_integrations"))) ) {
+        if ( (key.equals(strRes("piko_pref_app_version"))) || (key.equals(strRes("piko_title_piko_patches"))) ) {

This fix is fine.
After that, you can remove piko_title_piko_integrations

we could take it a step further and drop strRes entirely, replacing it with direct use of StringRef.

I'm not a maintainer, but I think so too.

Also, since repeatedly using "StringRef" will increase the number of characters, it would be better to static import app.morphe.extension.shared.StringRef.str.
Because this file particularly uses StringRef.str many times.

@krvstek

krvstek commented Apr 5, 2026

Copy link
Copy Markdown
Contributor Author

Please change this to piko_pref_import_success.
Sorry to keep asking for your help, but thank you in advance.

No worries at all. If there's anything else I missed, just let me know.

SettingsAboutFragment.java is not from Re*anced, it's genuinely from piko settings screen.
This is a remnant of the process that copies the version number when you pressed the Integration version.
The button has been removed, but it seems they forgot to remove the internal key (preference key) to distinguish buttons.

I meant specifically the term Integrations itself, which I assumed was a leftover from the RxVxnced project. Either way, key removed.

I'm not a maintainer, but I think so too.

Also, since repeatedly using "StringRef" will increase the number of characters, it would be better to static import app.morphe.extension.shared.StringRef.str. Because this file particularly uses StringRef.str many times.

Good idea, but let's not clutter this PR. I'll open a separate one if a maintainer gives the green light.

@krvstek

krvstek commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

Hey @swakwork, just a quick ping since this PR has been hanging around for a few months. Could you take a look and let me know if the code looks good to you?

I'd like to prepare some more PRs for the project, but the next one I'm working on directly depends on these changes getting merged first. Let me know if you need me to adjust anything!

@swakwork

swakwork commented May 6, 2026

Copy link
Copy Markdown
Collaborator

idk, for now I'm planning not to touch anything related to Twitter. Moreover, strings now come from Crowdln, so we might need to update there as well. So far now I'm not gonna pause it.
Will let you know based on the future of Twitter

@krvstek

krvstek commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

idk, for now I'm planning not to touch anything related to Twitter. Moreover, strings now come from Crowdln, so we might need to update there as well. So far now I'm not gonna pause it. Will let you know based on the future of Twitter

Makes total sense, especially with the recent changes making things so unpredictable.

Just a quick note regarding Crowdin, though: as far as I know, any string modifications pushed here should automatically sync with Crowdin via the GitHub workflow, so it shouldn't require any manual updating on our end.

@kitadai31

kitadai31 commented May 7, 2026

Copy link
Copy Markdown
Contributor

One thing to be aware of when merging this pull request is that after it is merged, the strings will be incomplete in that language until the translation is updated.
For example, the "Hide promoted posts" setting will temporarily display as "Promoted posts" until the translation in that language is corrected.
This becomes more problematic for languages ​​where there are no active translators.

Therefore, I propose the following merge strategy:

  1. Merge the latest Crowdin translations
  2. Release v3.3.0 stable
  3. Merge this PR immediately after 2.

This gives translators time to update their strings before Swak releases v3.4.0-dev.1.
(Because this pull request is refactor:, no new pre-release will be generated.
On the other hand, when this commit is pushed to the dev branch, the strings on Crowdin will automatically be updated. In other words, the translators can immediately begin updating the string.)

Furthermore, because stable versions are released less frequently, there is ample time before the next stable version (v3.4.0) is released. This reduces the likelihood of stable users encountering incomplete strings.

@krvstek

krvstek commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

idk, for now I'm planning not to touch anything related to Twitter. Moreover, strings now come from Crowdln, so we might need to update there as well. So far now I'm not gonna pause it. Will let you know based on the future of Twitter

Hey @swakwork, I see Twitter is getting some updates again in recent releases.

Since this is active, could we look into merging this refactor soon? I want to get it synced before the new changes create major conflicts.

Let me know if anything specific needs to be adapted on my end regarding the Crowdin migration you mentioned earlier.

@swakwork

Copy link
Copy Markdown
Collaborator

Hey @swakwork, I see Twitter is getting some updates again in recent releases.

Since this is active, could we look into merging this refactor soon? I want to get it synced before the new changes create major conflicts.

Will plan to include on the next Twitter release.

Let me know if anything specific needs to be adapted on my end regarding the Crowdin migration you mentioned earlier.

I'm don't know much about Crowdin. Do we need to include the new base keys to the project or will the translators themselves be adding them.

@krvstek

krvstek commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

I'm don't know much about Crowdin. Do we need to include the new base keys to the project or will the translators themselves be adding them.

From what I can see, this repo uses the exact same script as Morphe (Crowdin push/pull). So if there's a change in the strings, the push workflow replaces the source files, and the strings update automatically.

Someone correct me if I'm wrong.

@kitadai31

kitadai31 commented May 27, 2026

Copy link
Copy Markdown
Contributor

@swakwork

Will plan to include on the next Twitter release.

Then, could you merge this right now?

So the new strings will be reflected in Crowdin automatically, and we can get started on the translation.

Also, it would be helpful if you could make a brief announcement in the Telegram group Updates section.
Like:

For translation contributors: There have been significant changes to the Twitter strings. Dynamic string composition such as "Remove %s" and "Export %s" has been removed, and all strings can now be translated as independent strings. Many existing translations need correction. Please check on Crowdin.

@swakwork

Copy link
Copy Markdown
Collaborator

Will plan to include on the next Twitter release.

Then, could you merge this right now?

Well, by Twitter release I meant Piko update

So the new strings will be reflected in Crowdin automatically, and we can get started on the translation.

I dont mind merging it, now but the thing is I'm occupied, so I may not be able to support or do emergency release if needed.

Anyways let me merge it, I personally have not tested it, so if something goes wrong.... may god be with us 😌🙏

@swakwork swakwork merged commit 49a7c01 into crimera:dev May 27, 2026
1 check passed
@krvstek

krvstek commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Anyways let me merge it, I personally have not tested it, so if something goes wrong.... may god be with us 😌🙏

Thanks! No need to worry, this PR only changes strings and I've already personally tested it, so nothing should break.

@krvstek krvstek deleted the refactor branch May 27, 2026 14:41
@swakwork

Copy link
Copy Markdown
Collaborator

Thanks! No need to worry, this PR only changes strings and I've already personally tested it, so nothing should break.

Big thanks for the PR and so sorry for the delay

github-actions Bot pushed a commit that referenced this pull request May 27, 2026
## [3.5.0-dev.2](v3.5.0-dev.1...v3.5.0-dev.2) (2026-05-27)

### 🔧 Improvements

* **Twitter:** replace dynamic string composition with explicit string resources ([#845](#845)) ([49a7c01](49a7c01))
@kitadai31

Copy link
Copy Markdown
Contributor

Oh wait, did you run release workflow seriously?
I didn't mean that 😭
What I meant was to merge this PR into the dev branch right now so that the new strings is pushed to Crowdin, but not release the dev patches it yet 😭
This gave translators time to work on their translations before the "next Twitter release"

@kitadai31

kitadai31 commented May 27, 2026

Copy link
Copy Markdown
Contributor

Well, what's done is done

In my opinion, it would be better to remove the release, but of course it's up to you
EDITED: nvm I just saw your announcement in Tg. I think there is no need to remove.

Anyway, I'm sorry for involving you in this troublesome matter when your time is occupied 😥

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.

3 participants