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.

Sign up for GitHub

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

Jump to bottom

Refresh button #11

Merged
4 commits merged into from Mar 9, 2017
Merged

Refresh button #11

4 commits merged into from Mar 9, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 6, 2017

Added a refresh button, as mentioned here #8

Added a refresh button to bin the previous database, copy the new one…
9247ed0 
…, and reload it into the UI. Saves having to kill the app and re-open to see the latest changes. Should address "Issue #8 Reload Play Music Files"
@jcgruenhage
Copy link
Collaborator

I just tried it, a toast or something to inform the user about the changes might be useful, I don't see anything confirming the reload yet.

@jcgruenhage
Copy link
Collaborator

Also, since we are using JACK, the anonymous class might better be replaced by a lambda (or even a method reference, depending on where we would put such a method)

jcgruenhage
jcgruenhage requested changes Mar 8, 2017
View reviewed changes
Copy link
Collaborator

@jcgruenhage jcgruenhage left a comment

Choose a reason for hiding this comment

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

Some slight changes, but it is fine in general.

...icexporter/src/main/java/re/jcg/playmusicexporter/activities/MusicContainerListActivity.java
// Finds the search item and create the search view
MenuItem itemSearch = menu.findItem(R.id.action_search);
MenuItem itemSearch = menu. findItem(R.id.action_search);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why that space?

playmusicexporter/src/main/res/values/colors.xml Outdated
@@ -44,4 +44,6 @@
<color name="button_navigation_drawer_active">#11000000</color>
<color name="button_navigation_drawer_text">#88000000</color>
<color name="button_navigation_drawer_text_active">#ef6c00</color>
<color name="white">#ffffffff</color>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is that intended more that the rest?

playmusicexporter/src/main/res/values/strings.xml Outdated
@@ -146,4 +146,5 @@
<string name="file_handling_category">File Handling</string>
<string name="overwrite_summary">Always overwrite the exported file, even if it already exists.</string>
<string name="overwrite_title">Overwrite Existing Files</string>
<string name="action_refesh">Refresh</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is that intended more that the rest?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, Android Studio did that (added via the UI), I wonder if it's the eternal tabs vs spaces epic battle :-)

@jcgruenhage
Copy link
Collaborator

jcgruenhage commented Mar 8, 2017 •
edited

I might fix those myself, but I have no time right now, so if you do @mgillespie go ahead.
EDIT: This should also be rebased onto develop first

jcgruenhage
jcgruenhage requested changes Mar 9, 2017
View reviewed changes
Copy link
Collaborator

@jcgruenhage jcgruenhage left a comment

Choose a reason for hiding this comment

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

Thank you, overall, this is much better, only the few german strings that should be replaced keep me from merging it, I'll do that myself now, and merge it afterwards, or you can do that if you want :)

...icexporter/src/main/java/re/jcg/playmusicexporter/activities/MusicContainerListActivity.java Outdated
MenuItem itemRefreshLibrary = menu. findItem(R.id.action_refresh);
itemRefreshLibrary.setOnMenuItemClickListener(new MenuItem.OnMenuItemClickListener()
MenuItem itemRefreshLibrary = menu.findItem(R.id.action_refresh);
itemRefreshLibrary.setOnMenuItemClickListener(item ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

...icexporter/src/main/java/re/jcg/playmusicexporter/activities/MusicContainerListActivity.java Outdated
}
catch (NoSuperUserException | CouldNotOpenDatabaseException e)
{
Toast.makeText( this, R.string.dialog_superuser_access_denied_title, Toast.LENGTH_SHORT).show();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this quite misleading, if the thrown exception is a CouldNotOpenDatabaseException? I would not expect that to be thrown though, I've never seen it. Still, that might be something we should change later on, even though I wouldn't wait with merging the PR because of that.

playmusicexporter/src/main/res/values-de/strings.xml Outdated
@@ -106,4 +106,6 @@
<string name="file_handling_category">Dateiverarbeitung</string>
<string name="overwrite_summary">Überschreiben Sie die exportierte Datei immer, auch wenn sie bereits existiert.</string>
<string name="overwrite_title">Bestehende Dateien überschreiben.</string>
<string name="action_refesh">Erfrischen</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds weird, someting like "Erneut laden" (enl. load again) would be more fitting I think.

playmusicexporter/src/main/res/values-de/strings.xml Outdated
@@ -106,4 +106,6 @@
<string name="file_handling_category">Dateiverarbeitung</string>
<string name="overwrite_summary">Überschreiben Sie die exportierte Datei immer, auch wenn sie bereits existiert.</string>
<string name="overwrite_title">Bestehende Dateien überschreiben.</string>
<string name="action_refesh">Erfrischen</string>
<string name="databasereloaded">Reloaded Musikdatenbank</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reloaded is not german, I would suggest "Musikdatenbank neu geladen" or shorter "Datenbank erneuert"

playmusicexporter/src/main/res/values/colors.xml Outdated
@@ -44,6 +44,6 @@
<color name="button_navigation_drawer_active">#11000000</color>
<color name="button_navigation_drawer_text">#88000000</color>
<color name="button_navigation_drawer_text_active">#ef6c00</color>
<color name="white">#ffffffff</color>
<color name="white">#ffffffff</color>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

playmusicexporter/src/main/res/values/strings.xml Outdated
@@ -146,5 +146,6 @@
<string name="file_handling_category">File Handling</string>
<string name="overwrite_summary">Always overwrite the exported file, even if it already exists.</string>
<string name="overwrite_title">Overwrite Existing Files</string>
<string name="action_refesh">Refresh</string>
<string name="action_refesh">Refresh</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

jcgruenhage
jcgruenhage approved these changes Mar 9, 2017
View reviewed changes
Copy link
Collaborator

@jcgruenhage jcgruenhage left a comment

Choose a reason for hiding this comment

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

@mgillespie You can merge this now.

@ghost ghost merged commit 7dafd21 into develop Mar 9, 2017
@ghost ghost deleted the Refresh_Button branch March 9, 2017 15:09
@jcgruenhage jcgruenhage restored the Refresh_Button branch March 11, 2017 13:20
@jcgruenhage jcgruenhage deleted the Refresh_Button branch March 11, 2017 13:33
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@jcgruenhage