-
-
Notifications
You must be signed in to change notification settings - Fork 182
Added setting to toggle precise date as default display #156
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
Conversation
app/src/main/res/values/strings.xml
Outdated
| <string name="settings_appearance">Appearance</string> | ||
| <string name="setting_theme">Theme</string> | ||
| <string name="setting_key_theme">theme</string> | ||
| <string name="setting_precise_date">Precise date by default</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's unprecise about relative timestamps?
As I understand it the difference here is between relative and absolute timestamps. So for me it would make more sense naming this option Show absolute timestamps instead of Precise date by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree with you, to be honest I named it like that only because the boolean variable that was already there was named preciseDate and I just wanted to stay coherent.
If it's fine to everyone it can be changed to be relative/absolute time instead of precise.
| android:key="@string/setting_key_theme" | ||
| android:title="@string/setting_theme" /> | ||
|
|
||
| <CheckBoxPreference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for your contribution! Could we change this to a list preference with the label "time format" and values "absolute time" and "relative time"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll take care of this !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given there are only two choices wouldn't it be better to keep a checkbox? Check-box offers single-click solution while selecting from a list would require two clicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing settings isn't something that is done frequently, thus, I
prefer ui elements that are more descriptive than click efficient.
Presenting a user two options from which they can choose is IMO better than
hiding the default option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both of you on this, on the one hand as @jmattheis said you don't change settings frequently so an extra click for configuration isn't that much in my opinion. On the other hand, the default time displayed in the web app is always relative and there is no way to change it, making it the default one, so a checkbox overriding that default setup makes sense too.
I think I will do it with a ListPreference though, it's definitely more explicit, and user still have the possibility to toggle between the two modes just by clicking on the TextView.
| holder.setDateTime(message.message.getDate()); | ||
| holder.date.setOnClickListener((ignored) -> holder.switchPreciseDate()); | ||
| SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); | ||
| String timeFormat = prefs.getString(TIME_FORMAT_PREFS_KEY, TIME_FORMAT_RELATIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I've found one little bug. If no time format is configured, and the settings for time format is opened but dismissed, then the absolute time format is shown. The shared preferences get "Relative time" as value, and not the key relative_time_format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I set the default value in the ListPreference as the time_format_entry_relative instead of time_format_value_relative, my bad.
Thanks for the thorough review, it should be fine now.
jmattheis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you!


I've added a setting to fulfill one of the improvements proposed by @alekna in #145.
The setting allows the user to choose whether the default display mode for dates should be absolute or relative. Note that the toggle between the two modes is still working when the TextView is clicked.
Here is the settings activity:

And here is the main activity:

I've modified the way the precise date is formatted using the
DateFormatjava module.