Optimizing the uploader layout & taking user configured sorting into account #5
Conversation
|
|
||
| public class PreferenceManager { | ||
|
|
||
| public abstract class PreferenceManager { |
There was a problem hiding this comment.
To me this class name is super confusing, can you rename it to something like PreferenceHelpers ?
There was a problem hiding this comment.
Also I don't understand why this is in db package
There was a problem hiding this comment.
left this one open since it is a structural change
|
Very thorough review @przybylski thank you for that! 👍 I would like to start a discussion on this including @tobiasKaminsky in this. All the review comments are very legit and push for better code. The changes I made have been done with minimal changes in mind to be necessary to add the functionality but stay with the given/oC code structure to keep merges easy. It's tricky since easy merges mean we are stuck with the code as it is when we forked which might slow us down here and there. Diverge right at the start will make merge rather hard and might lead to errors down the road due to mistakes made while manually merging/adapting changes. Looking forward to your comments :) |
|
So let's wait what will @tobiasKaminsky say |
|
Sounds good to me. I am fine either way though but it should be a decision we all agree on :) |
AndyScherzinger
force-pushed
the
1193_uploader_layout
branch
from
17c887c
to
88314b8
Compare
|
Tricky one... |
|
Sounds good, thus I would propose the following steps:
Due to the possibility of differences in pace and future features the code base will diverge at some point anyways, but yes which should aim to get PRs merged and thus old branches removed and then move on from there. Sounds good to you guys? @nextcloud/android developers epsecially @przybylski and @tobiasKaminsky ? |
|
The plan sounds fine! (I need to avoid everything that l g t m can think I like this PR ;)) |
|
Well we could remove the thumbs up pattern so only shipit and L g M T would trigger the approval 😃 |
AndyScherzinger
force-pushed
the
1193_uploader_layout
branch
from
36a76a7
to
2432df0
Compare
|
Please review again. @tobiasKaminsky and @przybylski I incorporated all non-structural changes and also fixed some more layouting/data-display stuff so now all uploaders and the standard file list look exactly the same |
| android:layout_marginRight="4dp" | ||
| android:orientation="horizontal"> | ||
|
|
||
| <TextView |
There was a problem hiding this comment.
Wouldn't it be better to have one text view and format string when inserting?
There was a problem hiding this comment.
@przybylski I vote no since this would be the next change that doesn't add additional value but let's the code diverge from oC
|
opened an issue for the dimensions housekeeping: #12 to move stuff like this to dims.xml 😃 |
|
So are you fine with the changes @przybylski ? |
|
aside some structural changes LGTM |
|
Thanks a lot! Besides that looking forward to @tobiasKaminsky feedback especially on the matter on how (and when) to deal with technical debt and how to compile a list of PRs we want to integrate before we start larger refactorings. |
|
Sidenote... just realised that I (as in the person who opened the PR) can't even have a 👍 in a comment at all without that being tracked as a positive code review feedback... 😣 |
…ser configured sorting into account
now all file list view (uploader, upload external and standard file list have the same layout)
AndyScherzinger
force-pushed
the
1193_uploader_layout
branch
from
c21d80e
to
8073679
Compare
|
@AndyScherzinger of course it is. Just like LGTM needs it 👍 |
|
Sweet, so waiting for the build to finish and then I will merge :) |
NC version of @jancborchardt's original oC request 1193:
pinging @tobiasKaminsky @przybylski for the 2-thumbs-up code review
could be put into a post 1.0.0 release since it is a rather small change and easy to test (already tested by me)