Widget cleanup#298
Conversation
The widget will now check to see if the note exists before trying to display it. Fixes nextcloud#293. Moved opening of db from getViewAt. Fixed the widget's empty view.
korelstar
left a comment
korelstar
left a comment
There was a problem hiding this comment.
Are you done with your optimizations? There are some comments which indicate that this is not the case (see NotesListWidgetFactory regarding mAppWidgetId and SingleNoteWidgetFactory regarding "TODO Set loading view").
| Log.e(TAG, "Error: note not found"); | ||
| note = null; | ||
| } else { | ||
| note = db.getNote(noteID); |
There was a problem hiding this comment.
If the note exists (which is the default scenario) db.getNote(noteID) is executed twice. Since database queries are expensive, this should be optimized.
| @Override | ||
| public void onDestroy() { | ||
|
|
||
| db.close(); |
There was a problem hiding this comment.
Since NoteSQLiteOpenHelper is currently a singleton, it's dangerous to close the database from here. Theoretically, it's possible that another objects still wants to use the database. Hence, I suggest to remove this.
| android:layout_width="match_parent" | ||
| android:orientation="horizontal" | ||
| android:padding="4dp"> | ||
| android:padding="@dimen/widget_note_list_entry_layout_padding"> |
There was a problem hiding this comment.
What is the advantage of moving these dimensions to dimens.xml? As far as I can see, you are not re-using any of them! From my point of view, this makes everything much more complex ...
| android:layout_height="match_parent" | ||
| android:paddingLeft="6dp" | ||
| android:paddingStart="@dimen/widget_note_list_entry_padding_start" | ||
| android:paddingEnd="@dimen/widget_note_list_entry_padding_end" |
There was a problem hiding this comment.
Why do use different dimensions for padding left and start (respective right and end)? These values should be the same and therefore the dimension keys, too (if used at all, see other comment). I don't think, that the file values-v17/dimens.xml is needed.
|
Hello Korelstar, I was intending on using mAppWidgetId in NotesListWidgetFactory but have removed it for now because it will be a while before I get around to it. The TODO for the loading view is a reminder but I will remove it if you prefer. I noticed that the "Loading" text isn't formatted great but it only appears on my phone for a split second every so often. I'll work on the getNote() operation in SingleNoteWidgetFactory. I was trying to avoid using try-catch. I'll remove the call to db.close() also, that hadn't occured to me. I moved the dimensions to a separate file as I was under the impression it's best practice to allow for different layout sizes on different devices (with varying display sizes). I agree I could have used the same key name for the left/right and start/end dimensions but I thought it might be confusing to have |
No, that's fine.
Just save the result as local variable.
Okay, but I still think, that maintainability would be increased if you use one single dimension-key for all usages that belong to each other. E.g., introduce a dimension
If you name it
I don't understand this. If you use
Isn't this already the case when the app is executed on a device supporting the respective API? |
* Removed unused imports * Improvements to accessibility and internationalisation * Removed hard coded dimensions in layouts * Condensed dimensions definitions. Alignment problem.
dan0xii
force-pushed
the
widget_cleanup
branch
from
7da8662 to
c5e69ac
Compare
|
Take a look at the latest revision. I think I have ironed out all of the issues. |
korelstar
left a comment
korelstar
left a comment
There was a problem hiding this comment.
Perfect! Thank you very much, @dan0xii
@stefan-niedermann I would merge this if you don't raise any objections.
|
Nope i'm fine with the changes, merge :) |
3 participants
General improvements to widget code
This includes the changes from #297 so it will be worth waiting until that's agreed before reviewing this PR.