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

redesign application ui #132

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

redesign application ui #132

wants to merge 34 commits into from

Conversation

icai
Copy link
Contributor

@icai icai commented Jun 16, 2024

redesign application ui

Copy link
Owner

@pilot51 pilot51 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and especially the new translation!

This is by far the largest PR ever for Voice Notify and it may take some time before I'm comfortable merging it in. I'm also asking the community what they think of the redesign, which may have an impact on what changes I allow.

There are a number of issues below that I would like you to resolve before I will approve, as well as some nitpicks that aren't necessary for approval.

To help me and the community better understand the changes, please edit the PR description with a summary of changes and include screenshots of the redesigns.

Also please set your formatter to use tab indentation for this project so it is consistent. Regardless of your preference, I think we can agree there's nothing worse than mixed indentation.

app/build.gradle.kts Outdated Show resolved Hide resolved
app/schemas/com.pilot51.voicenotify.db.AppDatabase/1.json Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/java/com/pilot51/voicenotify/MainScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/pilot51/voicenotify/MainScreen.kt Outdated Show resolved Hide resolved



val ClearBlue = Color(0xFF286EFF)
Copy link
Owner

@pilot51 pilot51 Jun 23, 2024

Choose a reason for hiding this comment

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

This is harder to read in the dark theme than the previous primary color (0xFF1CB7D5). I suggest using separate colors for each theme, preferably keeping the previous colors which I chose based on the icon to stick with a common theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot_20240626_135808_com.android.settings.jpg

Screenshot_20240626_135824_com.pilot51.voicenotify.debug.jpg

make it like android 14 systemui

Copy link
Owner

Choose a reason for hiding this comment

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

The screenshot below is what I'm talking about for both color issues.
In this case, ClearBlue is being used for the button text color, which does not have good contrast and can be especially hard to read in certain lighting conditions.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change to dialog background and rewrite the design.


private val DarkColorScheme = darkColorScheme(
primary = ClearBlue,
secondary = Color(0xFF2A54A5),
Copy link
Owner

Choose a reason for hiding this comment

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

The text replace dialog uses secondary for the description and this color makes it hard to read.

app/src/main/java/com/pilot51/voicenotify/db/App.kt Outdated Show resolved Hide resolved
app/src/main/java/com/pilot51/voicenotify/ui/TextField.kt Outdated Show resolved Hide resolved
@pilot51
Copy link
Owner

pilot51 commented Jun 26, 2024

It may be a good idea to create a separate PR for just the translation. That should be quick to push through. The redesign is holding it back.

app/src/main/java/com/pilot51/voicenotify/db/App.kt Outdated Show resolved Hide resolved
implementation("androidx.compose.material:material-icons-extended-android:1.6.7")
implementation("androidx.compose.ui:ui-tooling-preview:1.6.7")
debugImplementation("androidx.compose.ui:ui-tooling:1.6.7")
implementation("androidx.compose.material3:material3:1.3.0-beta04")
Copy link
Owner

Choose a reason for hiding this comment

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

Build error: This breaks PreferenceDialogs.rememberTimePickerState() because TimePickerState.Saver() was removed in beta02. I suggest reverting to beta01 and I'll handle the migration.

app/src/main/java/com/pilot51/voicenotify/db/App.kt Outdated Show resolved Hide resolved
@pilot51
Copy link
Owner

pilot51 commented Aug 25, 2024

I find that the Ignore None switch is confusing. It would be less confusing to call it "Enable All", but I think it would be more intuitive to keep ignore all/none as separate actions, as it was originally, so it can double as a "reset all to default". If we use a switch, I think it should reflect appDefaultEnable. One thing that was missing from the original design was an indication of what the default is.

Suggestions:

  1. Go back to the old way, with Ignore All and Ignore None options.
  2. Change the switch to "Enable by Default" and have it only toggle appDefaultEnable without toggling any apps. Add the separate Ignore All and Ignore None options. Changing the default won't need the confirmation, but the bulk actions will.
  3. Change the switch to "Enable by Default" and show a confirmation dialog that provides options to reset all apps to the new default, only change the default, or cancel.

On another note, I was waiting for all my concerns with this PR to be resolved so I could include it in v1.4.0. Unfortunately that's holding things up more than I had anticipated, so for now I'll plan on getting this in v1.5.0. That includes the translation unless you split that into another PR or add it through Weblate.

@pilot51 pilot51 added this to the v1.5.0 milestone Aug 25, 2024
@icai
Copy link
Contributor Author

icai commented Sep 6, 2024

@pilot51 AI translation should be faster. Copy the default Value string.xml file, prompt:{content} translate all value to chinese, keep the order, return all of the content. And the Suggestions I will review again.
image.

@pilot51
Copy link
Owner

pilot51 commented Oct 10, 2024

I tried using AI translations once and someone who knew the language that I translated to told me to never do that again.
Often the translations don't come out right because it doesn't understand the context.
Since I am personally unable to verify a translation because I am not familiar with the language, I will not be using AI translation.
I don't care if contributing translators use AI, but I have to trust that they understand the language and proofread it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants