Brought projects to current ver. Android Studio and Gradle#40
Brought projects to current ver. Android Studio and Gradle#40gregko wants to merge 4 commits intopingpongboss:masterfrom
Conversation
Also made some small code corrections to remove depreciated or removed calls and classes, e.g. using now NotificationCompat.Builder
pingpongboss
left a comment
pingpongboss
left a comment
There was a problem hiding this comment.
Hey! First of all, thanks for this PR. This will really modernize this library which hasn't been receiving a lot of love in recent years.
I hope you're willing to work with me through this PR. The main theme of my review will be to minimize the number of changes you make. Things like unnecessary package name changes, folder structure changes, gradle configurations, etc, will be reverted so we get a fairly clean PR. Let me know if you have any questions.
| @@ -0,0 +1,22 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
this is a contentious subject, but I'd like to remove all of the contents of .idea and make it gitignored. This is especially true now that you've helpfully made the project gradle friendly.
| @@ -0,0 +1,33 @@ | |||
| apply plugin: 'com.android.library' | |||
There was a problem hiding this comment.
I see you've renamed library/ to StandOutLib/
please rename back to library/ to keep the directory structure as before
| } | ||
|
|
||
| dependencies { | ||
| compile fileTree(dir: 'libs', include: ['*.jar']) |
| androidTestCompile('com.android.support.test.espresso:espresso-core:2.2.2', { | ||
| exclude group: 'com.android.support', module: 'support-annotations' | ||
| }) | ||
| compile 'com.android.support:appcompat-v7:25.3.1' |
There was a problem hiding this comment.
Does the library need an appcompat dependency?
| Notification notification = new Notification(icon, tickerText, when); | ||
| notification.setLatestEventInfo(c, contentTitle, contentText, | ||
| contentIntent); | ||
| Notification notification = new NotificationCompat.Builder(this) |
There was a problem hiding this comment.
use the application context unless using this solves some issue. Same below.
| @Override | ||
| protected void onCreate(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(R.layout.activity_main); |
There was a problem hiding this comment.
do you need to set a content view when the activity isn't meant to display anything?
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| @@ -0,0 +1,7 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
are these used anywhere? Can they be removed?
Same question for all other values you've added under res/
| @@ -0,0 +1,17 @@ | |||
| package hyperionics.com.standouttest; | |||
|
|
|||
| @@ -0,0 +1 @@ | |||
| include ':app', ':StandOutLib' | |||
There was a problem hiding this comment.
you'll want to add floatingfolders here when you make it a module
|
Hi Boss,
Please go ahead and do the changes as you see fit, it’s your library. I’m really too busy to make so many small changes. A few quick answers:
AppCompat dependency – the “new Notification()” simply does not work, one has to use NotificationCompat.Builder, and use AppCompat for compatibility with older versions of Android. I did not change anything else in the library code. However I checked now and see that Notification.Builder was added in API 11, and since the minSdkVersion is set to 15, you may change it to just use Notification.Builder instead and remove appcompat. Oh, another problem, Notification.Builder().setVisibility() requires API 21, unless you use NotificationCompat.Builder()
Using application context – good idea, else we may face memory leaks.
“library” module name – my app uses a lot of library projects, so having one of them named simply “library” would be really confusing for me.
Units tests and some XML lines you object to – they are created automatically by Android Studio for each new project, feel free to remove them, but I find unit tests and instrumentation test skeletons handy when a need arises to test something.
Wrong package names in some places – sorry about that. Initially I just wanted to make it work asap. Then refactored the package name under Android Studio, and it missed a few places (e.g. the manifest and build.gradle). I’ll correct these in my GitHub fork.
|
Also using application context in creating notifications, to avoid possible memory leaks, restored the test app to show MultiWindow and WidgetsWindow by default (as in the original project), simplified build.gradle, some cosmetic changes.
|
In a new commit in my fork I corrected the wrong package names, also changed to use app context in creating notifications, removed unnecessary empty layout for the MainActivity, restored the test app to show MutiWindow and WidgetsWindow by default, as in the original, simplified the app build.gradle and made some other small cosmetic changes. Not sending another pull request at this time, as it seems you'll want to tweak the cosmetics more. |
|
Thanks gregko. Sounds like you don't have time to clean this PR up. Let's leave this PR here for now then. If I get any time, I may take over this PR in the future. Thanks, |
Made displayWidth and displayHeight static, added updateScreenSize(context) method, to be called from StandOutWindow service upon configuration change.
ACTION_TOGGLE_VIS and the code to handle it is for toggling visibility from the persisten notification alternative, instead of having additional notification to show the window. To disable hidden notification, slightly modified the code to ignore it, if the overwritten getHiddenNotificationIntent() returns null. To handle orientation changes, added onConfigurationChanged() call, which calls Window.updateScreenSize().
2 participants
Hello!
Thank you for such an elegant and compact, yet powerful and well documented library. I had troubles building it with the old tools, so re-worked projects structure to work with the latest versions of Android Studio and Gradle. The test examples needed to deal with the new way of handling screen overlay permission under Android 6 and higher, which I added to the test app main activity class.
Also made some small code corrections to remove depreciated or removed calls and classes, e.g. using now NotificationCompat.Builder.
Consider merging it into your base fork, may help others quickly start with your library. I need it for my @voice Aloud Reader app to make a floating Play/Pause button and maybe a few other buttons on a floating toolbar.