Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Replace the Firefox word in all strings with a placeholder #22189

Closed
Amejia481 opened this issue Oct 27, 2021 · 12 comments · Fixed by #23168
Closed

Replace the Firefox word in all strings with a placeholder #22189

Amejia481 opened this issue Oct 27, 2021 · 12 comments · Fixed by #23168
Assignees
Labels
eng:qa:verified QA Verified Hacktoberfest Highlights issues for Hacktoberfest contributors to help us! help wanted Help wanted from a contributor. More complex than good first issue.
Milestone

Comments

@Amejia481
Copy link
Contributor

Amejia481 commented Oct 27, 2021

We would like to replace the word "Firefox" in all our strings with a string placeholder (only on /app/src/main/res/values/strings.xml). For example:

This is what we have: 😞

    <string name="synced_tabs_no_tabs">You don’t have any tabs open in Firefox on your other devices.</string>

This is what we need: 😄
The string above, we want replace the word "Firefox" with a placeholder that we will replace in code:

    <string name="synced_tabs_no_tabs" moz:removedIn="95" tools:ignore="UnusedResources">You don’t have any tabs open in Firefox on your other devices.</string>
    <string name="synced_tabs_no_tabs_2">You don’t have any tabs open in %1$s on your other devices.</string>

Replace each reference of the string in code using the string placeholder app_name:

        val appName = resources.getString(R.string.app_name)
        description.text = getString(
            R.string.synced_tabs_no_tabs_2, appName
        )

From the code above, you can notice:

  1. We don't actually update the string synced_tabs_no_tabs we created a new string adding a _# (a underscore with number, starting on 2), and added the properties moz:removedIn="95" tools:ignore="UnusedResources" to synced_tabs_no_tabs. We do this, for limitation described here.
  2. In code we replace the place the placeholder with the string app_name, we also have to make sure we replace all reference of the old string (synced_tabs_no_tabs) with the new string (synced_tabs_no_tabs_2) both in code and XML layouts.

For more context see mozilla-l10n/android-l10n#393 (comment)

┆Issue is synchronized with this Jira Task

@Amejia481 Amejia481 added help wanted Help wanted from a contributor. More complex than good first issue. Hacktoberfest Highlights issues for Hacktoberfest contributors to help us! labels Oct 27, 2021
@github-actions github-actions bot added the needs:triage Issue needs triage label Oct 27, 2021
@pocmo pocmo removed the needs:triage Issue needs triage label Oct 27, 2021
@S10MC2015
Copy link

Would you still want the string changed where it says "intentionally hardcoded" and where it says "Firefox Account"/"Firefox Sync"?

@Amejia481
Copy link
Contributor Author

Yeah, we will need to change the comments. Thanks for pointing that out.

@grigoryk
Copy link
Contributor

grigoryk commented Nov 5, 2021

@Amejia481 I'm assuming this is what you meant, but just to clarify:
I think in case of intentionally hardcoded strings, e.g. something like Firefox Account or Firefox Sync, we should leave them as-is. That is, we don't have a Firefox Preview Sync service.

@Amejia481
Copy link
Contributor Author

@grigoryk thanks for clarifying, yeah, you are right. @S10MC2015 as @grigoryk mentioned we want to keep the ones that are referring to specific Firefox services as they are. We only want to update strings that are related to the app's name.

@Alexandru2909
Copy link
Contributor

Hi, I would like to work on this issue!

@Amejia481
Copy link
Contributor Author

@Alexandru2909 thanks for interesting on this bug, lets just confirm with @S10MC2015. if we don't get any update after a day, please feel free to work on it, as it's been a long time since the last update.

@S10MC2015
Copy link

@Alexandru2909 can work on it.

Sorry I haven't done any work. I don't think I'll have the time to work on for a while either.

@Alexandru2909 Alexandru2909 self-assigned this Jan 11, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Jan 12, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Jan 13, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Feb 8, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Feb 10, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Feb 16, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Feb 16, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Feb 17, 2022
@gabrielluong gabrielluong added this to the 99 milestone Feb 18, 2022
@mergify mergify bot closed this as completed in #23168 Feb 18, 2022
mcarare added a commit to mcarare/fenix that referenced this issue Mar 1, 2022
@sflorean
Copy link
Contributor

sflorean commented Mar 2, 2022

Tested this on Nightly 99 and here are the results:

Case 1

  • signed with the same Firefox account on Nightly 2 devices with no open tabs: "You don't have any tabs open in Firefox Nightly on your other devices" is displayed.

Case 2

  • signed with the same Firefox account on Nightly and 2 different profiles of Firefox Release with no open tabs: "You don't have any tabs open in Firefox Nightly on your other devices" is displayed.

For case 2, the placeholder shouldn't be Firefox only? since the other tabs are on 2 different profiles of Firefox Release.

@Amejia481 @Alexandru2909

@Amejia481
Copy link
Contributor Author

@sflorean 👋🏽

Thanks for taking a look!
Yes, I think so as it looks we are referring to tabs in any Firefox,@Alexandru2909 could also help to clarify.

@Alexandru2909
Copy link
Contributor

I agree, this seems to be the case. Could the same reasoning be applied to "Sync Firefox between devices"? or is it better to use app_name here?

@Amejia481
Copy link
Contributor Author

Yes, I guess 😅 , it's a bit tricky but I think it was the intention of the message.

Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Mar 7, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Mar 7, 2022
Alexandru2909 pushed a commit to Alexandru2909/fenix that referenced this issue Mar 9, 2022
@Alexandru2909 Alexandru2909 added the eng:qa:needed QA Needed label Mar 10, 2022
pedroldk pushed a commit to pedroldk/fenix that referenced this issue Mar 22, 2022
pedroldk pushed a commit to pedroldk/fenix that referenced this issue Mar 22, 2022
@SoftVision-LorandJanos
Copy link

Verified as fixed on the latest Nightly 100.0a1 (2022-04-01) build.
Device used: Oppo Reno 6 (Android 12).

@SoftVision-LorandJanos SoftVision-LorandJanos added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Apr 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng:qa:verified QA Verified Hacktoberfest Highlights issues for Hacktoberfest contributors to help us! help wanted Help wanted from a contributor. More complex than good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants