Skip to content

Conversation

@AnEnigmaticBug
Copy link
Contributor

Fixes #1082

Changes: WelcomeFragment has been split into a fdroid and a playstore variant. This has been done because the fdroid version doesn't need a ViewModel and any location services but the playstore version does.

The fdroid flavor had a redundant ViewModel and it's code was confusing because the ViewModel was used for entirely different purposes in the two flavors.

Making two flavor specific modules(one for fdroid and one for playstore) allowed us the split the WelcomeFragment itself.

This has the benefit that the code in WelcomeFragment now reflects the features of the flavor and the redundant ViewModel is no longer needed.

@iamareebjamal
Copy link
Member

It's OK but the separation has now changed from ViewModel based to Fragment based. I personally don't like entire fragments being duplicated because you need to make changes in both if there's any change.

In reality, there was no difference in the fragment, but in the location module. Hence, only the location module should have been split. Yes, this reduced a no-op version of WelcomeViewModel, but increased an entire fragment. Now, Welcome Fragment is a very small fragment, but imagine a large one. If you even change a view ID in the layout of one flavor, the app will crash in the second variant. And the worst part is that it will crash during runtime.

Whereas, if we split by module, yes, we have to replicate a no=op module but we did not have to deal with Android's framework classes/layout splitting which is a PITA.

Also, if we change one implementation of our module, we'll have compilation error in the second one which is good and we can make the same change there. I may merge it because this is a trivial case but I don't think splitting by fragment is a great idea for future. It may reduce duplication but makes maintenance of fragments worse.

The question reduces to, would you want to duplicate fragments, or self written components

@AnEnigmaticBug
Copy link
Contributor Author

I see you points and agree that we should carefully ponder splitting Fragments. I also think that this should be allowed for larger Fragments only after a careful weighing of pros and cons.

But IMHO(as you pointed out) the small size of WelcomeFragment is what makes it practical to split the Fragment in this case.

There is only a single common layout. Since the R file is generated at/before build time, if we change the id of a view in the layout, any issues related to a non-existent id should appear as a build error because R.id.<old_id> would not exist.

My main reason for doing this change was the fact that the fdroid version of WelcomeFragment had a lot of code which was used in a non-intuitive way. Also, it made the presence of WelcomeViewModel a necessity. And that ViewModel exposed LiveData which didn't make sense for the fdroid flavor. It felt to me that we were treating fdroid version as an afterthought. I thought that in this case, the benefits of splitting the Fragment were more significant than the cons.

I understand that this is very subjective and that your concern is as valid as mine.

@iamareebjamal
Copy link
Member

Fix the Travis build please

WelcomeFragment has been split into a fdroid and a playstore variant.
This has been done because the fdroid version doesn't need a ViewModel
and any location services but the playstore version does.

The fdroid flavor had a redundant ViewModel and it's code was confusing
because the ViewModel was used for entirely different purposes in the
two flavors.

Making two flavor specific modules(one for fdroid and one for playstore)
allowed us the split the WelcomeFragment itself.

This has the benefit that the code in WelcomeFragment now reflects the
features of the flavor and the redundant ViewModel is no longer needed.

Fixes: fossasia#1082
@AnEnigmaticBug
Copy link
Contributor Author

I've fixed the Travis build. But Codacy is failing because it wants documentation on WelcomeFragment

@iamareebjamal iamareebjamal merged commit 48f4e42 into fossasia:development Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants