Added the ability to limit video quality if using mobile data. #1339
Conversation
* Added a dropdown to video & audio settings * Changes to ListHelper: ** Limits resolution when code requests the default video resolution ** Limits audio bitrate when code requests the default audio bitrate ** Removed some dead code and did some cleanup ** Make methods private/protected to help understand what was in use ** The code now chooses one format over an other using a simple raking system defined in array constants. I realized I needed to do this in order to choose the most efficient video stream. I did my best to evaluate the video and audio formats based on quality and efficiency. It's not an exact science. ** Made changes to the tests to support my changes
| <!-- Limit mobile data usage --> | ||
| <string name="limit_mobile_data_usage_key" translatable="false">limit_mobile_data_usage</string> | ||
| <string name="limit_mobile_data_usage_value" translatable="false">@string/limit_data_usage_none_key</string> | ||
| <string-array name="limit_data_usage_description_list"> |
There was a problem hiding this comment.
The description should be put into strings.xml not into the settings_kes.xml file
There was a problem hiding this comment.
I thought strings.xml was for stuff that required translation? None of these contain user facing labels. I was mimicking default_resolution_key, default_resolution_value, and resolution_list_values as listed above. Do you want me to move limit_data_usage_description_list into strings.xml? Should I include resolution_list_values also then?
There was a problem hiding this comment.
You can mark them as nontranslatable than they will not get translated.
I want the settings_keys.xml just contain keys.
| <item>240p</item> | ||
| <item>144p</item> | ||
| </string-array> | ||
| <string-array name="limit_data_usage_values_list"> |
There was a problem hiding this comment.
Don't we have this list for the other resolution select already?
There was a problem hiding this comment.
Almost, resolution_list_values is mostly identical except for the first item: "Best resolution" vs "No limit". I was also bothered by the duplication but I didn't know of a way to share some of the items without more refactoring and I was trying to avoid too many code changes for my first push. We may also want to consider a "No video" item at some point.
| // If the user has chosen to limit resolution to conserve mobile data | ||
| // usage then we should also limit our audio usage. | ||
| int result; | ||
| if (isLimitingDataUsage(context)) { |
There was a problem hiding this comment.
why not
if (isLimitingDataUsage(context)) {
return getMostCompactAudioIndex(defaultFormat, audioStreams);
} else {
return getHighestQualityAudioIndex(defaultFormat, audioStreams);
}
There was a problem hiding this comment.
I think because my original changes were more complicated (multiple limiting levels). Some people prefer one return at the end of a method so I kept it. It doesn't matter to me either way, I'll throw in the returns.
There was a problem hiding this comment.
I am an R-Value/functional fan, so I think return would fit well here :)
| SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(context); | ||
|
|
||
| // Load the prefered resolution otherwise the best available | ||
| String resolution = preferences != null ? preferences.getString(context.getString(key), |
There was a problem hiding this comment.
Please format this line like this:
String resolution = preferences != null
? preferences.getString(context.getString(key), context.getString(value))
: context.getString(R.string.best_resolution_key);
theScrabi
changed the title
- Moved non-key strings from string_keys.xml to strings.xml - Code style changes - Replaced a hard coded key string with resource constant
|
If ur done please remove the wip sign |
Zenopheus
changed the title
|
all right nice contribution thank you :) |
Fixes #1259
This limits the user's default video resolution when not connected to WiFi. The user can choose "No Limit"(default) or a specified resolution ("144p" for example). When active the audio stream with the lowest bitrate is also chosen. This differs from the proposed design due to complexity concerns. For example, supporting a "No Video" option would have required a lot of changes to be implemented cleanly.
Changes to Settings video & audio settings
Changes to ListHelper:
I carefully read the contribution guidelines and agree to them.