-
-
Notifications
You must be signed in to change notification settings - Fork 727
view model to load page in PageFragment #5762
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
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # app/src/main/java/org/wikipedia/page/PageFragment.kt
… api calls - codes fixes
… api calls - codes fixes - adds categories db code, fixes refresh calls and code for sending events
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.
Just did a quick scan of this PR and it looks pretty good! 👍
One issue I found:
- When loading an article that has a lead image, the footer does not get loaded correctly, and it shows up after refreshing the page.
app/src/main/java/org/wikipedia/page/pageload/PageDataFetcher.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/wikipedia/page/pageload/PageDataFetcher.kt
Outdated
Show resolved
Hide resolved
handlePageSummary(pageSummary, request) | ||
|
||
val watchStatus = watchStatusDeferred.await() | ||
handleWatchStatusAndFetchCategories(watchStatus, request.title) |
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.
The fetchWatchStatus
already contains the categories response, so this function should either pass the categories
from the fetchWatchStatus
directly, or fetch it only if the categories
is empty.
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.
done, the current logic fetches data based on the applied conditions.
val Factory: ViewModelProvider.Factory = viewModelFactory { | ||
initializer { | ||
val app = this[APPLICATION_KEY] as WikipediaApp | ||
PageLoadViewModel(app) | ||
} | ||
} | ||
} |
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.
Could you please explain it a bit, and what's the difference if we set a val app = WikipediaApp.instance
directly in the view model 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.
Functionally, there won’t be a difference since WikipediaApp instance is an object and can be accessed easily everywhere, but ideally, any external dependencies should be injected into the ViewModel even the dataFetcher that i have. I will remove this since this can create confusion.
the read more is loading lazily for me, i can a slight delay though |
As per discussion, the issue arises rarely and its difficult to reproduce, will monitor this. |
What does this do?
Phabricator:
https://phabricator.wikimedia.org/T303945