-
Notifications
You must be signed in to change notification settings - Fork 14
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
Implement new report submittion fragment #129
Conversation
|
||
override fun convert(file: File): String { | ||
return ImageUtils.convertToBase64EncodedString(context, file) | ||
?: throw RuntimeException("Failed to convert photo ${file.path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should fail silently when impossible to convert photo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some scenarios when photo conversion will fail in our case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- OOM when Glide is converting file to bitmap because of huge bounds:
final int requiredWidth = 1600;
final int requiredHeight = 1600;
- file not found for some reason
- because shit happens :)
9686e19
to
700bfbe
Compare
Any chance for a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing 👍 Really nice MVP implementation.
Left some comments and got one exception while testing.
@@ -37,4 +37,8 @@ abstract class BaseFragment : Fragment() { | |||
|
|||
analytics.trackCurrentFragment(activity, this) | |||
} | |||
|
|||
override fun onDestroyView() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this one so in super child classes there were lint errors, that super is not called...
|
||
return Observable.from(data.photoUrls) | ||
.map { photoConverter.convert(it) } | ||
.filter { it != null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There won't null from photoConverter::convert
due to RuntimeException
if image can't be converterd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so we need to decide what to do. With that. Exception?
return | ||
} | ||
|
||
val current = personalDataInteractor.getPersonalData() ?: Profile() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to get current user profile. Just pass required values to contructor (preferably convert Profile
to KT data class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Each profile value pass to view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean:
val profile = Profile(reportData.name,
LocalDate.parse(reportData.dateOfBirth),
reportData.email,
reportData.phone)
personalDataInteractor.storePersonalData(profile)
In this case new setters are not needed.
@@ -53,8 +47,16 @@ public String getMobilePhone() { | |||
return mobilePhone; | |||
} | |||
|
|||
public static Profile returnProfile(Context con) { | |||
return SharedPrefsManager.getInstance(con).getUserProfile(); | |||
public void setBirthday(LocalDate birthday) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not needed you have all required data to pass to constructor
return this | ||
} | ||
|
||
fun <E> validate(validator: FieldAwareValidator<E>): FieldAwareValidator<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used
@@ -11,4 +11,8 @@ public ApiRequest(ApiMethod method, P params) { | |||
this.id = method.getId(); | |||
this.params = params; | |||
} | |||
|
|||
public P getParams() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only used for tests use VisibleForTesting
reportType = reportType, | ||
description = report_problem_description.text.toString(), | ||
address = report_problem_location.text.toString(), | ||
latitude = locationCords?.latitude, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use LatLng
instead of two doubles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid using Android
dependencies in models.
@@ -2,20 +2,32 @@ package lt.vilnius.tvarkau.activity | |||
|
|||
import android.os.Bundle | |||
import lt.vilnius.tvarkau.BaseActivity | |||
import lt.vilnius.tvarkau.fragments.NewReportFragment | |||
import lt.vilnius.tvarkau.fragments.ReportTypeListFragment | |||
|
|||
class ReportRegistrationActivity : BaseActivity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing this PR got:
12-13 17:56:25.045 31875-31875/lt.vilnius.tvarkau E/WindowManager: android.view.WindowLeaked: Activity lt.vilnius.tvarkau.activity.ReportRegistrationActivity has leaked window DecorView@485b2d3[] that was originally added here
at android.view.ViewRootImpl.<init>(ViewRootImpl.java:418)
at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:331)
at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:93)
at android.app.Dialog.show(Dialog.java:322)
at lt.vilnius.tvarkau.fragments.NewReportFragment.showProgress(NewReportFragment.kt:545)
at lt.vilnius.tvarkau.mvp.presenters.NewReportPresenterImpl$submitProblem$3.call(NewReportPresenterImpl.kt:49)
at rx.internal.operators.SingleDoOnSubscribe.call(SingleDoOnSubscribe.java:43)
at rx.internal.operators.SingleDoOnSubscribe.call(SingleDoOnSubscribe.java:28)
at rx.internal.operators.SingleDoOnUnsubscribe.call(SingleDoOnUnsubscribe.java:42)
at rx.internal.operators.SingleDoOnUnsubscribe.call(SingleDoOnUnsubscribe.java:28)
at rx.Single.subscribe(Single.java:1876)
at rx.Single.subscribe(Single.java:1686)
at lt.vilnius.tvarkau.mvp.presenters.NewReportPresenterImpl.submitProblem(NewReportPresenterImpl.kt:51)
at lt.vilnius.tvarkau.fragments.NewReportFragment.onOptionsItemSelected(NewReportFragment.kt:156)
at android.support.v4.app.Fragment.performOptionsItemSelected(Fragment.java:2212)
at android.support.v4.app.FragmentManagerImpl.dispatchOptionsItemSelected(FragmentManager.java:2295)
at android.support.v4.app.FragmentController.dispatchOptionsItemSelected(FragmentController.java:353)
at android.support.v4.app.FragmentActivity.onMenuItemSelected(FragmentActivity.java:414)
at android.support.v7.app.AppCompatActivity.onMenuItemSelected(AppCompatActivity.java:198)
at android.support.v7.view.WindowCallbackWrapper.onMenuItemSelected(WindowCallbackWrapper.java:107)
at android.support.v7.view.WindowCallbackWrapper.onMenuItemSelected(WindowCallbackWrapper.java:107)
at android.support.v7.app.ToolbarActionBar$2.onMenuItemClick(ToolbarActionBar.java:69)
at android.support.v7.widget.Toolbar$1.onMenuItemClick(Toolbar.java:206)
at android.support.v7.widget.ActionMenuView$MenuBuilderCallback.onMenuItemSelected(ActionMenuView.java:777)
at android.support.v7.view.menu.MenuBuilder.dispatchMenuItemSelected(MenuBuilder.java:817)
at android.support.v7.view.menu.MenuItemImpl.invoke(MenuItemImpl.java:156)
at android.support.v7.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:964)
at android.support.v7.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:954)
at android.support.v7.widget.ActionMenuView.invokeItem(ActionMenuView.java:624)
at android.support.v7.view.menu.ActionMenuItemView.onClick(ActionMenuItemView.java:157)
at android.view.View.performClick(View.java:5637)
at android.view.View$PerformClick.run(View.java:22429)
at android.os.Handler.handleCallback(Handler.java:751)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:154)
at android.app.ActivityThread.main(ActivityThread.java:6119)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)
Device Nexus 5x newest Android version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, good catch
super.onSaveInstanceState(outState) | ||
outState?.let { | ||
it.putParcelable(SAVE_LOCATION, locationCords) | ||
it.putSerializable(SAVE_PHOTOS, imageFiles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have Icepick
dependecy. No need to manually save instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work with KT :/ Or I failed to grasp how should I make it work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup there is issue frankiesardo/icepick#47 with workaround to make KT work with Icepick. IMHO it's better to not use icepick with KT
android:id="@+id/report_problem_submitter_name" | ||
style="@style/NewProblemField" | ||
android:hint="@string/profile_hint_name" | ||
android:inputType="textPersonName"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use textPersonName|textCapWords
I think if at least one field was filled and back is pressed it should ask user if he really wants to exit. |
See travis for failing tests |
Also IMHO we need events to track user behaviour in this screen. We should track at least submitted report. |
I'll add tracking with separate PR. It's too big already :) |
Looks awesome. |
New report submission using fragments + MVP + KT.
Any input is appreciated.
TODO: