search

lemberg / connfa-android

Open Source Android app for Conferences and Events
http://connfa.com
Apache License 2.0
100 stars 111 forks source link

Batch of crash fixes and tweaks #21

Closed rock3r closed 7 years ago

rock3r commented 7 years ago

This PR contains the following changes:

rock3r commented 7 years ago

There is a lot of changes I'm afraid, had to do a pass of reformatting of the app module to have uniform code style.

TarasKunyk commented 7 years ago

Hello, thanks for your update. We have a few notes/questions on the current source base:

  1. There is database crash on the migration procedure
  2. Timezone dialog is shown each time details screen is dismissed
  3. You have added social media activity instead of the fragment. What's the point of such update?
  4. You have hardcoded toolbar height in the resources. Wouldn't ?android:attr/actionBarSize constant from the compatibility library better suit this situation?
  5. social_query is defined with build properties instead of being loaded from server. This limits app flexibility a bit. don't you think so?
rock3r commented 7 years ago

@TarasKunyk here's the answers

  1. Please create an issue on the fork so I can look into it (there is no "previous" version of this and we didn't change the DB, so I'm not sure what you're migrating from)
  2. Ditto, please create an issue
  3. I'm trying to gradually move the app to the one-activity-per screen model, which makes a thousand times easier to deal with lifecycle and callbacks. For example, with that model it's almost impossible to have the broken behaviour we currently have with multiwindow on Android N.
  4. That's a private resource, you should not be using it directly (the IDE will even warn you about that) as it might change/go away at any time
  5. I've never seen the need for that, if you find yourselves doing that all the time we can look into that later on (but again, never seen a conference changing their social hashtag anyway, so it's probably YAGNI)
rostyslawbulych commented 7 years ago

Hi @rock3r! Seems that your update will result in a significant app update. And it will take some time to make it stable. So what do you think about moving changes to separate branch named connfa_v2 and go on working with it. While previous app version will be published on connfa_v1 and master branches since it's currently stable. What do you think?

rock3r commented 7 years ago

Hi @rostyslawbulych! Sure, works for me. If you create the branch (branching off of github-master) I can change the target for this PR and rebase all our future work against that.

For more details on what we're planning to do, please take a look at the issues and boards on our fork. Feel free to get in touch if you have questions/suggestions, you should have my details.

TarasKunyk commented 7 years ago
  1. You are right. That's some old bug, I had pushed fix to the repo 2.-
  2. Sidebar pattern does require fragments usage for the best user experience. Do you have any alternatives?
  3. Anyway, that value isn't constant. It's different in landscape mode and for tablets. So we have to declare all values possible. And it's defined within compatibility library so it isn't OS-dependend and safe to use.
  4. You are right, that value probably will never be updated. But sometimes it isn't defined at the moment of application release (on iOS app release takes almost a week) so we do need this feature and would like t keep both app versions as close as possible in terms of functionality.
rock3r commented 7 years ago

Hey there @TarasKunyk!

  1. 👍
  2. 👍
  3. The navigation drawer pattern can be achieved with a seamless UX when using one-activity-per-page too; all apps I have worked on so far have the nav drawer and don't use fragments. It's all a matter of doing overridePendingTransition() at the right place
  4. You're right about needing different values but that only applies if you explicitly support landscape (we currently don't) and tablet-specific layouts (we don't either). If/when we manage to put those in then we'll need to add the overloads in values-land and values-sw600dp, but we don't need them yet. Actually the fact the support lib does that regardless of us not supporting either case is a problem as we have 64 dp toolbars on tablets but the rest of the layout is a phone layout. I double checked and it's not a private resource anymore in the latest support library version, so at least that should be ok now, but I'd still not do it for the aforementioned issues.
    1. Ok, what if we had as a feature to implement to try and fetch that data from the backend and rely on the built-in value (as implemented here) if there's none? Can make sure it's in the next bundle PR.

PS I'm changing the base of this PR to the v2 branch

TarasKunyk commented 7 years ago
  1. Such workaround won't allow us to reproduce same user experience as fragments provide: Screen animation will be a bit different from the original one. Plus you won't be able to add hamburger menu button to the subscreens and handle it correctly. We would like to keep existing sidebar functionality. But could you provide us a few samples of your apps using your navigation approach?

Additionally I would like to note that we could apply sidebar navigation pattern to activity-based app using deprecated ActivityGroup API. Sample project is attached to the post. NestedActivity.zip

rock3r commented 7 years ago

Well, the functionality can be preserved. That's the whole point of me proposing it, if it weren't possible I wouldn't.

As for the ActivityGroup, it seems a rather dangerous path to go down. At that point I'd rather keep the fragments.