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

Avoid duplicate files #1168

Merged
merged 25 commits into from
Dec 9, 2015
Merged

Avoid duplicate files #1168

merged 25 commits into from
Dec 9, 2015

Conversation

tobiasKaminsky
Copy link
Contributor

@@ -11,4 +11,18 @@

</declare-styleable>

<string-array name="pref_behaviour_entries">
<item>do nothing</item>

Choose a reason for hiding this comment

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

I think it is better to define a string per item and use them in the string-array item instead of using a literal.
Besides, we are using Transifex to translate into different languages and it takes the file strings.xml to do the translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right, I have changed it.

@tobiasKaminsky
Copy link
Contributor Author

@ALL: Please review

@tobiasKaminsky
Copy link
Contributor Author

behaviour
Behaviour now has a summary.

@tobiasKaminsky
Copy link
Contributor Author

Behaviour now has a default value.

@tobiasKaminsky
Copy link
Contributor Author

@AndyScherzinger Regarding the dialog, it is not happening to me, so I cannot verify if.
But this is a ListPreference not a Dialog.
Do you have an idea?

@tobiasKaminsky
Copy link
Contributor Author

upload

@AndyScherzinger 16dp as padding on the left. Do I have to put this also on the right?

@AndyScherzinger
Copy link
Contributor

Yes, it should also be on the right even tough I guess we will never hit that end :)

@AndyScherzinger
Copy link
Contributor

As for the dialog, not really. This only happens on pre-lollipop devices where one needs to explizitely replace dialogs with the appCompat ones. Not sure if and how that could be achieved for a preference screen and its list preference in particular.

@AndyScherzinger
Copy link
Contributor

I pushed a custom list preference implementation that should take care of it.
The Dialog looks fine now. I tested it on an Android 15 emulator which doesn't change the value on the preference screen right after choosing an option but it gets persisted (close preferences, open them again and you see the new value). I did the same test with the original ListPreference with the same results, so this might be due to the emulator.

@davivel @masensio can you by any change test this on a real device to make sure my latest push to this branch works for the List Preference?

The replacement in the UI works perfectly on my nexus5 btw.

@AndyScherzinger
Copy link
Contributor

Added some more styleing, see screenshot
device-2015-09-27-204223

@AndyScherzinger
Copy link
Contributor

@jancborchardt any comment on the paddings? The bottom elements are now in line with the material style guide (16dp) and thus are perfectly aligned with the icon in the action bar. Unfortunately the icons of the list items are slightly larger than the others and are thus center aligned with the actionbar icons (on the left) and the bottom "bar" icons. It looks a bit out of balance but I don't have any idea how to "fix" this visual inbalance :(

@jospoortvliet
Copy link

I believe that Jan is on holiday so we'll have to figure things out by ourselves for a bit ;-)

I like how it looks now - perhaps we can merge it this way and create an issue to fix it later on. That creates a nice junior job for somebody new to our Android app!

@AndyScherzinger
Copy link
Contributor

Sweet! Than it is already in place in this PR.
@tobiasKaminsky Hope I is okay that I pushed this to your PR/branch :)

@tobiasKaminsky
Copy link
Contributor Author

@AndyScherzinger Thank you for your help with this PR.
It is great to see cooperation on an issue :)

@AndyScherzinger
Copy link
Contributor

@masensio the line isn't doubled (#1168 (comment)) I just added a grey line as a separator between the list and the bottom button area, so in some cases while scrolling it might leave the impression that there are 2 lines which is 1 for the bottom area and one is the list divider 😄

@masensio
Copy link

@AndyScherzinger, ok I understood this line is a separator.
I think when this line seems a duplicated line is strange, maybe an option would be that this line was a little wider to be different than the others separators. It is only an idea, maybe this also seems strange.

@AndyScherzinger
Copy link
Contributor

@jancborchardt what do you think, regarding @masensio's idea for the separating line? (see screenshot #1168 (comment)). Making the line slightly thicker would be easy but would probably introduce a new style since then we might have to adapt it so other screens with button bars on the bottom. Not sure what would be the best way to handle this.

Log_OC.d(TAG, "upload file and do nothing");
i.putExtra(FileUploader.KEY_LOCAL_BEHAVIOUR, FileUploader.LOCAL_BEHAVIOUR_FORGET);
} else if (behaviour.equalsIgnoreCase("COPY")) {
i.putExtra(FileUploader.KEY_LOCAL_BEHAVIOUR, FileUploader.LOCAL_BEHAVIOUR_COPY);
Copy link

Choose a reason for hiding this comment

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

If I'm not wrong, is this option accesible at this point?
Same comment for behaviour.equalsIgnoreCase("DELETE")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

COPY and DELETE exist no longer, so we can remove them.

@masensio
Copy link

masensio commented Dec 3, 2015

Hi @tobiasKaminsky,
thanks for your update.
We need to test a bit before merging.

For me the code is good. :)

masensio pushed a commit that referenced this pull request Dec 9, 2015
@masensio masensio merged commit 896e485 into master Dec 9, 2015
@masensio masensio deleted the avoidDuplicateFiles branch December 9, 2015 08:28
@masensio
Copy link

masensio commented Dec 9, 2015

PR Accepted :)

@tobiasKaminsky
Copy link
Contributor Author

👍
This is great :)

@AndyScherzinger
Copy link
Contributor

Awesome! 👍

@enoch85
Copy link
Member

enoch85 commented Dec 13, 2015

👍 wohoow! Just tested on beta, works great! Thank you!

@jospoortvliet
Copy link

WHOOHOOOO

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.