Conversation
|
I've tested it quickly and it works. However, there are many issues I'd like to be fixed before this gets merged. I suspect this has been done largely with the help of AI, yet you haven't mentioned it. In fact, both the PR and the (single!) commit have no description. Are you willing to make several improvements? Or is this just a feature you made for yourself and thought of contributing back? |
|
@maltaisn Yes, it's done with AI help for a personal project. I don't have the time right now to make changes. Please change it around to get it merged. |
|
This is your PR, if you want it merged you can make the changes. I'll do a review when/if you have time. |
|
@maltaisn What are the issues you wanted fixed? I will try to fix them. |
|
|
||
| /** | ||
| * Status of the note, i.e. its location in the user interface. | ||
| * Note status, i.e. its location in the user interface. |
| * 0 is the default color. | ||
| */ | ||
| @ColumnInfo(name = "color", defaultValue = "0") | ||
| val color: Int = 0, |
There was a problem hiding this comment.
I would prefer the color to be an enum rather than an int. Int is meant more to represent integer numerical quantities.
| * Notes with [status] set to [NoteStatus.ACTIVE] should be pinned or unpinned. | ||
| * Other notes should be set to [PinnedStatus.CANT_PIN]. | ||
| */ | ||
|
|
| } | ||
|
|
||
| debugRequire(status != NoteStatus.DELETED || reminder == null) { | ||
| require(status != NoteStatus.DELETED || reminder == null) { |
There was a problem hiding this comment.
Why were these changed to require? This applies at other locations too.
| R.string.action_share, | ||
| R.drawable.ic_share, | ||
| true, | ||
| false, |
There was a problem hiding this comment.
You add one action but 3 actions that could be shown in toolbar now can't. Why?
There was a problem hiding this comment.
I changed it because I was having trouble running tests, but I'll undo the changes to the existing toolbar buttons
| <color name="note_color_3">#2C2A3E</color> | ||
| <color name="note_color_4">#2A3B2E</color> | ||
| <color name="note_color_5">#3D2E29</color> | ||
| </resources> |
There was a problem hiding this comment.
The colors look fine when used as a background, but they don't in the color picker:
The same is true in dark theme. I would prefer to show colors at higher saturation in the picker.
| android:focusable="true" | ||
| android:contentDescription="@string/action_none" /> | ||
|
|
||
| <Space android:layout_width="16dp" android:layout_height="0dp" /> |
There was a problem hiding this comment.
Using Space is very antiquated. If you change to constraint layout you won't need it anymore.
| <dimen name="input_dialog_title_min_height">420dp</dimen> | ||
|
|
||
| <integer name="edit_actions_in_toolbar">5</integer> | ||
| <integer name="edit_actions_in_toolbar">8</integer> |
There was a problem hiding this comment.
This explains why the toolbar was so crowded. This change is not related to adding colors.
| <item name="android:minWidth">@dimen/toolbar_action_button_min_width</item> | ||
| <item name="android:paddingStart">@dimen/toolbar_action_button_padding_horizontal</item> | ||
| <item name="android:paddingEnd">@dimen/toolbar_action_button_padding_horizontal</item> | ||
| </style> |
There was a problem hiding this comment.
What does this do and why was it changed? It's not related to colors.
|
|
||
| companion object { | ||
| private const val KEY_SELECTED_COLOR = "selected_color" | ||
| const val TAG = "color_picker_dialog" |
There was a problem hiding this comment.
Nitpick: parent fragments should be responsible for setting the tag. For example a fragment could have multiple color picker fragments and use the tag to differentiate them, like it is the case with confirm dialog currently.
|
General comments:
|
2 participants
No description provided.