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

Kotlin and a bunch of other stuff #95

Merged
merged 63 commits into from Feb 22, 2017
Merged

Kotlin and a bunch of other stuff #95

merged 63 commits into from Feb 22, 2017

Conversation

smichel17
Copy link
Member

@smichel17 smichel17 commented Nov 1, 2016

This is the continuation of #88. It fixes some or all of #36, #70, #88, #90, #94.

This has become a really big PR, but it should be clear what I've changed if you look at it commit by commit (with the exception of ed27961, because android studio added the new kotlin files to git without asking).

TODO:

  • Bugfix: Crashes when trying to get location permission on emulator, api 21.
    • This turned out to maybe just be an emulator bug. The emulator didn't have a network location provider. Keeping the fix in, just in case, and because it includes better logging.
  • Bugfix: toggling the theme scrolls back to the top of the activity instead of maintaining scroll position.
  • Re-add a pref just for updating the location. I tried to find a way to replace it with a toggle entirely, but there's just no way. If you give a prominent notice that you're updating the location, it feels too invasive ("Why does Red Moon need to know my location right now?) but if you don't give a prominent notice, the app feels like it's missing a way to update the location.
    • We can probably fix this when we add a way to manually set the location, in another sub-screen. But that's more work than I want to do right now.
  • Re-enable location; pull location updating out into its own service so the app doesn't need to stay in the foreground while the location updates.
  • Re-enable ShadesPresenter (the fragment approach plus null safety breaks it) / replace it with eventbus
  • Better interface for ScreenFilterService
  • Bug: Turning on Secure Suspend always pauses. Introduced in 8d97285
  • Bug: Preview doesn't work if you start changing setting while filter is turning off.
    • And other weirdness with turning the filter on and off the first time you open the app
  • Bug: Changing settings doesn't cause the profile spinner to change to 'custom'
  • Allow turning the filter back on while it's in the middle of turning off
    • There is a more elegant fix to this that doesn't cause it to flash completely off, which involves moving openScreenFilter() and closeScreenFilter() into ScreenFilterView. I just want to get this merged, though, so I'll do the easy/brittle fix for now.
  • Bug: Timer doesn't go on
    • ScheduleNextCommand seems to work correctly, but onRecieve never gets called. The receiver had accidentally been removed from the manifest during the rename.
  • Bug: Sometimes two screen filters can get started.
    • This can also be reproduced by repeatedly using the toggle widget, with amusing results.
      • Toggle on works, but toggle off does not (although Red Moon thinks it does), so the next time you turn it on, now you've got two filters running.
      • If you put a phone in developer mode, you can see that when started this way, the filter does not show up as a service, but as a background process. When you swipe it away in the app history, the filter will turn off.
    • Toggling on seems to work fine. Toggling off is not actually turning the filter off.
  • Clean up android studio warnings and remove unused imports
  • Add my name to copyright notice in modified files

Not going to do these now:

  • show the on & off times in the Automatic toggle summary
    • We'll need to add additional strings.
    • I'm unsure what's needed to work with RTL locales.
    • If we end up doing More complex toggling #106, not sure how we'd display that.
    • How big of a deal is it that we don't display them?
  • Bug: crashes when turning on filter if permission to draw over other apps is not granted:
    • This is also present in the live app (Fail gracefully if overlay permission is revoked #85) so I'm going to say it's not in scope of this MR.
    • Solution will be to do the checks for permission in the screenfilterpresenter. If the check fails, post a message to the event bus. That way, if the app is open, it'll receive the event and can launch a dialog asking for permissions; if the app isn't open, it'll fail silently. We can use this approach for other permissions, too, if appropriate.
  • Make getting a new location more reliable.
  • Clean up compilation warnings / add eventbus subscriber index / modify Lint for kotlin
    • These are nice-to-haves, but they don't prevent the application from running. I'd like to do them before the next release, but I just want to get merged to master so I don't have to keep maintaining what's essentially a fork. Plus when I know I'm making individual MRs and not working on one big one, my commits are much cleaner.
  • Bugfix: Secure Suspend doesn't work on Galaxy S6, api 23
    • The current app monitoring thread fails to recognize the package installer as secured.
    • Suspect this is a Samsung problem, as it works on an api 23 emulated device.
    • Verified that this also happens on the live app, so this is not a regression -> wontfix

@smichel17
Copy link
Member Author

Note: the android studio kotlin plugin will work much better if you update gradle (#92)

@smichel17 smichel17 mentioned this pull request Nov 1, 2016
Add a switch to toggle between sun and custom, not yet functional.
…s stupid.

Automatic conversion also automatically stages the file, making it impossible
to for me to separate changes I made before converting (which were going to
be in a separate commit) and the conversion itself. Thanks, Obama.
@smichel17 smichel17 changed the title Add kotlin and convert settingsModel Kotlin and automatic toggle Nov 11, 2016
@smichel17 smichel17 changed the title Kotlin and automatic toggle Kotlin and a bunch of other stuff Nov 11, 2016
@smichel17 smichel17 changed the title Kotlin and a bunch of other stuff WIP: Kotlin and a bunch of other stuff Nov 11, 2016
@smichel17 smichel17 changed the base branch from master to feature_stop_state November 11, 2016 16:04
@smichel17 smichel17 changed the base branch from feature_stop_state to master November 11, 2016 16:05
@smichel17
Copy link
Member Author

smichel17 commented Nov 14, 2016

@raatmarien I just did a fair amount of reorganization, both UI and code.

On the UI side, I moved the automatic toggle prefs into their own sub-category, and made automatic toggle on/off is now styled the same as secure suspend.

On the code side, automatic toggle prefs are now their own fragment. Secure suspend is its own fragment, too. I'm going to strip things out of the main activity until it's just a shell which coordinates swapping between fragments (at this point, the only thing left is to change the switch back to a fab, that lives in the filter fragment. I've also moved a bunch of code that lived in custom preferences into the fragments. The automatic suspend fragment uses more xml now and we re-use the xml for on/off in the Time Toggle fragment.

The motivation for these changes was that a lot of places had to call back to a parent activity or fragment -- that dependency meant they weren't really independent to be moved around at will. At that point, they're just adding a lot of boilerplate code, which was an added burden to understand. There was also a little bit of duplicate code. My goal is to reduce the number of references that different parts of the application have to keep to each other.

Not yet implemented: [moved into top comment]

@smichel17
Copy link
Member Author

@raatmarien I added eventbus. For now, I just replaced onSettingsChangedListener, because that was the easiest to implement, even though there wasn't much of a gain from doing so (the real gain is that I think eventbus will simplify dealing with state in ScreenFilterPresenter).

@raatmarien
Copy link
Member

Thanks for all the work you're doing on this! Eventbus does look nice.

Do you think it would be best for me to wait with testing/reviewing this until later, since it is a WIP, or would it be best to try and give some input now?

@smichel17
Copy link
Member Author

smichel17 commented Feb 10, 2017

@raatmarien Does the ShortcutToggleActivity actually do anything? If it does, I can't figure out what.

Can it be removed?

@raatmarien
Copy link
Member

Hmm, the static method toggleAndFinish is used by the TileService here, but I don't think any of the other parts are needed anymore.

@smichel17 smichel17 changed the title WIP: Kotlin and a bunch of other stuff Kotlin and a bunch of other stuff Feb 12, 2017
@smichel17
Copy link
Member Author

smichel17 commented Feb 12, 2017

@raatmarien I'm sure there are bugs I haven't found yet, but I've fixed all the known regressions from the current state, so I'll consider this MR complete.

There's been quite a few major refactorings (ScreenFilterService, ScreenFilterPresenter, LocationUpdateService, and FilterFragment are the biggest ones), so let me know if there's anything you want me to walk you through or changes I should make before you're comfortable merging.

@smichel17
Copy link
Member Author

smichel17 commented Feb 12, 2017

I should probably do testing on a variety of api levels. So far I've only tested on 23 (Galaxy s6) and 24 (emulated nexus 5).

@smichel17
Copy link
Member Author

@raatmarien Did some more testing, have fixed all the regressions I've been able to find. What should I do to get this merged (aside from, wait for you to have time)?

The 'Visit project page' menu item also opened a new email, it now only
opens the project page.
When the TimeToggleActivity or SecureSuspendActivity where closed with
the arrow in the action bar, the wrong animation would display, making
it seem that the main activity was opened over the subactivity. Now the
close activity animation is displayed, so it looks like you are
returning to the main activity (which you are, the subactivities aren't
kept 'under' the main activity).

To get android to display this animation correctly I used a sort of a
workaround, overriding the functionality of the arrow in the action bar
with calling finish() on the activity. I'm not aware of any other way to
do this, so I think this should do fine.
The app now asks for access to coarse location when the 'Times from
location' switch is enabled and it doesn't have the permission, instead
of silently failing.
The maybeInitializedSwitch thing may be an ugly hack, but I don't know
enough about Kotlin to find out the correct way. As far as I know it is
null save and will only change it if the switch is already set up.
@raatmarien
Copy link
Member

@smichel17 I finally got the time to take a look and I'm really impressed. The UX and UI feel very polished, great work on this PR 👍 !

I've added a few commits to fix some of the rough edges I encountered, but I think this is rock solid overall and definitely ready to be merged! Thanks a lot for all these contributions you make to Red Moon!

@raatmarien raatmarien merged commit f46a864 into LibreShift:master Feb 22, 2017
@raatmarien
Copy link
Member

I've sent you a collaborator invite for this repo, so you can push directly to master or another branch and tag/close issues if you want, since you have been a great contributor to Red Moon 😄

About releasing a new update of Red Moon: I was thinking that we could release 3.0.0 with these changes and everything that is on master right now. The only think I want to fix before that is using PRECISE_LOCATION instead of COARSE_LOCATION to (hopefully) fix #102, #107 and similar issues. Do you have any ideas of what more should be done before releasing the update?

@smichel17
Copy link
Member Author

I missed this reply, oops!

I've sent you a collaborator invite for this repo, so you can push directly to master or another branch and tag/close issues if you want, since you have been a great contributor to Red Moon 😄

Thank you, it's very helpful!

About releasing a new update of Red Moon: I was thinking that we could release 3.0.0 with these changes and everything that is on master right now. The only think I want to fix before that is using PRECISE_LOCATION instead of COARSE_LOCATION to (hopefully) fix #102, #107 and similar issues. Do you have any ideas of what more should be done before releasing the update?

I fixed up a bunch of other little things. Otherwise, seems good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants