Override on destory to retain fragments #2979
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
Codecov Report
@@ Coverage Diff @@
## refactor_uploads #2979 +/- ##
===================================================
- Coverage 4.36% 4.36% -0.01%
===================================================
Files 257 257
Lines 12358 12367 +9
Branches 1059 1059
===================================================
Hits 540 540
- Misses 11780 11789 +9
Partials 38 38
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Please update the commit message to reflect the content of the patch.
Also, it would be helpful to explain why this is needed, maybe by adding a link to an issue or similar for context
https://chris.beams.io/posts/git-commit/#why-not-how
Reviewed with ❤️ by PullRequest
| } | ||
|
|
||
| @Override | ||
| public void destroyItem(ViewGroup container, int position, Object object) { |
There was a problem hiding this comment.
This commit does not give context about why disabling the item destruction is needed, it would be good to at least document that.
The items are going to hold images right? Are you worried that keeping the images (or whatever is in the fragments) around can cause too much memory pressure? Why was a FragmentStatePagerAdapter used in the first place?
Looking at its documentation, there is a mention to FragmentPagerAdapter that does not do any memory clearing. Maybe refactor to use that as base class instead if you really want to disable that? The memory management is a defining feature of FragmentStatePagerAdapter, if would probably be easier to change the base class, and reduce the risks of introducing hard to track bugs.
|
|
||
| import android.os.Bundle; | ||
| import android.text.TextUtils; | ||
| import android.util.Log; |
| /** | ||
| * Attaches the scale change listener to the image view | ||
| */ | ||
| private void attachImageViewScaleChangeListener() { |
There was a problem hiding this comment.
Rename the method to something like attachCollapseMediaDetailsOnImageInteractionListener() ?
Ideally a reader of the init() method should know right away that the purpose of the listener is to collapse MediaDetails. Currently the method name does not say anything about its purpose.
| } | ||
|
|
||
| /** | ||
| * show hide media detail based on |
There was a problem hiding this comment.
Documentation nits:
-
Javadoc should use Sentence case, terminate with a full stop. (styleguide)
-
@paramstarts a specific block describing the argument, if you want to refer to an argument in the javadoc body, use @code like:/** Show or hide media detail based on {@code shouldExpand}. */
Generally try to optimise the documentation writing for how it renders in the IDE (e.g. Ctrl-Q in Android Studio) or when the HTML javadoc is generated
| btnCopyPreviousTitleDesc.setVisibility(View.VISIBLE); | ||
| } | ||
|
|
||
| attachImageViewScaleChangeListener(); |
There was a problem hiding this comment.
This change seems irrelevant to the rest of the patch. Mention it in the commit message? Or separate to its own patch?
It's a best practice to make each commit have an easily understandable single purpose:
https://github.com/trein/dev-best-practices/wiki/Git-Commit-Best-Practices#commit-related-changes
| private void attachImageViewScaleChangeListener() { | ||
| photoViewBackgroundImage.setOnScaleChangeListener( | ||
| (scaleFactor, focusX, focusY) -> { | ||
| //Whenever the uses plays with the image, lets collapse the media detail container |
There was a problem hiding this comment.
s/uses/user
Also, this is actually a comment that would be nice to include in the Javadoc, to give a reason why this method is there.
2 participants
Retain upload fragment states when pressed back