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

Feature/strings updates #1659

Merged
merged 37 commits into from
Sep 9, 2024
Merged

Conversation

ThomasSession
Copy link
Collaborator

Sorting out String issues, as per tasks mentioned here: https://discord.com/channels/408081923835953153/1280040841800581172

And tickets here: https://optf.atlassian.net/browse/SES-2690

ThomasSession and others added 30 commits September 6, 2024 08:34
Making sure the summary was displaye properly for drop down preferences
Latest strings
Removed the LEGACY disappearing messages
New title for Share activity (also formatted the lines)
The control message is now clickable when the phone toggle is disabled or when the microphone permission is not given
Updated the avatar picker to match the designs. Had to rewrite it in Compose and moved the logic to the VM.
We could moev the whole settings page to compose in another step now that we have the VM set up.
Copy link
Collaborator

@AL-Session AL-Session left a comment

Choose a reason for hiding this comment

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

Found a few things we might want to adjust - nothing major, but will hold off approving until we've discussed.

@@ -139,7 +137,7 @@ internal fun LandingScreen(
R.string.onboardingBubbleWelcomeToSession -> {
Phrase.from(stringResource(item.stringId))
.put(APP_NAME_KEY, stringResource(R.string.app_name))
.put(EMOJI_KEY, WAVING_HAND_EMOJI)
.put(EMOJI_KEY, "\uD83D\uDC4B") // this hardcoded emoji might be moved to NonTranslatableConstants eventually
.format().toString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how demoting this emoji from a named constant that explains what it is to two code-points with no explanation improves anything.

var id: Long = 0

// this can throw an error
id = retryIfNeeded(4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the 4 in retryIfNeeded(4) go into a constant like MAX_PROFILE_PICTURE_UPLOAD_ATTEMPTS or similar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if the upload fails and exceeds the retry count then what's going to happen when we use the id below? It looks like an exception might be thrown, so should we catch & log something? Or not update the preference value? Or both?

@@ -192,8 +193,9 @@
<string name="clearMessagesNoteToSelfDescription">Are you sure you want to clear all Note to Self messages from your device?</string>
<string name="close">Close</string>
<string name="closeWindow">Close Window</string>
<string name="communityBanDescription">This will ban the selected user from this Community. Are you sure you want to continue?</string>
<string name="commitHashDesktop">Commit Hash: {hash}</string>
Copy link
Collaborator

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 this Desktop string - we just put the first 6 digits of the hash in without explanation in the SettingsActivity at the bottom.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll see if the script can be updated to remove iOS and desktop strings

@@ -363,6 +366,12 @@
<item quantity="one">And %1$d other has reacted %2$s to this message.</item>
<item quantity="other">And %1$d others have reacted %2$s to this message.</item>
</plurals>
<string name="emojiReactsHoverNameDesktop">{name} reacted with {emoji_name}</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

More Desktop strings that can be removed. Anything with Desktop in it should likely go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll see if the script can be updated to remove iOS and desktop strings

@@ -523,7 +523,7 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
Column {
// add the debug menu in non release builds
if (BuildConfig.BUILD_TYPE != "release") {
LargeItemButton(DEBUG_MENU, R.drawable.ic_settings) { push<DebugActivity>() }
LargeItemButton("Debug Menu", R.drawable.ic_settings) { push<DebugActivity>() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not convinced this is beneficial, but up to y'all..

val userPublicKey = getUserPublicKey() ?: return

val mmsDb = DatabaseComponent.get(context).mmsDatabase()
val message = IncomingMediaMessage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we converted IncomingMediaMessage to Kotlin we could have named arguments, because that constructor is absolutely grim and invites mistakes!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that is a nice update that we can work on post strings

libsession/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@@ -149,12 +138,14 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
if (result.resultCode != Activity.RESULT_OK) return@registerForActivityResult

val outputFile = Uri.fromFile(File(cacheDir, "cropped"))
val inputFile: Uri? = result.data?.data ?: tempFile?.let(Uri::fromFile)
val inputFile: Uri? = result.data?.data ?: viewModel.getTempFile()?.let(Uri::fromFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While unlikely, inputFile could be null here because createTempFile can fail - so perhaps log something rather than proceed to cropImage with a null file if inputFile is null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this follows the logic that was currently in place. I'm not exactly sure how it can reach a state where the VM's temp file is null. We could maybe make sure it never is. But the input file here already expects the Uri to be nullable

Copy link
Collaborator

@SessionHero01 SessionHero01 left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasSession ThomasSession merged commit a6efb43 into release/1.20.0 Sep 9, 2024
0 of 2 checks passed
@ThomasSession ThomasSession deleted the feature/strings-updates branch September 9, 2024 04:31
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