Update resume state when you press back when watching movie #1751
Conversation
| @@ -1,6 +1,13 @@ | |||
| import "pkg:/source/utils/misc.bs" | |||
| import "pkg:/source/utils/config.bs" | |||
|
|
|||
| ' Valid lost status values for view | |||
| enum LoadStatus | |||
There was a problem hiding this comment.
Do we need the enum? It looks like we could use m.isFirstRun like the other views. m.loadStatus is only checked on L114 and doesn't seem to care about the 3 different states. Only if it's the view's firstRun or not
There was a problem hiding this comment.
This one was a little different than the other view, it had a preexisting OnScreenShow() function with code that needs to run every time the function is called. I did originally have isFirstRun in there, but it felt like the code didn't give as much context to why and what we were truly testing. By using the enum, I felt the flow of the function was easier and quicker to follow.
There was a problem hiding this comment.
If the code needed more context, why not just add a comment?
I disagree that it makes the function easier and quicker to follow. I also think there is value in having some kind of standard like isFirstRun for all views.
There was a problem hiding this comment.
If the code needed more context, why not just add a comment?
Clean code standards favor code itself providing context and being as clear as possible. In fact, the book says "Comments are liars!"
Confidentially, between working on the tv episode PR and this movie PR, I finished a PluralSight class that pushed had to not use binary values for state/status values as they lock you into a single true/false condition, and are not future proof, meaning if in 6 months you need something to use the next status, you now must create a new variable to use or refactor your code. Instead, they push for using enums just like this so if in the future we need to check 2nd load, 5th load, 8th load, we won't need to make invidual is8thLoad, is5thLoad variables, we can simply add a new enum status values and use it.
There was a problem hiding this comment.
This is a post summarizing some of the clean code comments chapter: https://medium.com/codex/clean-code-comments-833e11a706dc
The item pertaining to why use code itself to provide context and not adding a comment is "Don’t Use a Comment When You Can Use a Function or a Variable"
There was a problem hiding this comment.
Clean code standards favor code itself providing context and being as clear as possible. In fact, the book says "Comments are liars!"
I don't understand why you felt it needed more context in the first place. It looks fine to me 🤷
if m.isFirstRun
m.isFirstRun = false
else
m.top.refreshMovieDetailsData = not m.top.refreshMovieDetailsData
end if
This app is over 5 years old and I don't believe anyone has ever followed that philosophy. There are plenty of comments in the code. I also don't remember this new philosophy being discussed with the team or on github or anywhere else or I would have voiced my opinion about not wanting to convert our app.
Shouldn't this be discussed first? Shouldn't this be it's own issue and PR and not just slipped in with a bug fix? Shouldn't all views be using enums instead of isFirstRun?
We have a hard enough time getting people to code for our app. Not everyone understands the full context of all the code. Some people code one feature and then leave. Making our policy be "Comments are liars!" is counter productive to our app as a whole. We will end up with less PRs or less useful comments in those PRs not to mention it will add more burden to the people doing the reviews.
We talked in the past about documenting our code standards. I believe we should finish that before we start adding to the standards. That way at least there's a reference for new coders and reviewers as well as forcing new code standards to be documented which would hopefully encourage a discussion first.
There was a problem hiding this comment.
I don't understand why you felt it needed more context in the first place. It looks fine to me 🤷
It's likely due to the training I just completed. Like I said, I didn't do it this way for TV, but when I hit this point for movies, using the single variable felt like the wrong solution. I'm sure it's because they harped so much on the benefits of avoiding binary variables for status and states.
I would have voiced my opinion about not wanting to convert our app.
I too don't want to convert our app, but I don't think we should force all new code to blindly follow the same code style of 5 years ago. 5 years ago we didn't have access to enums. Now that we do, why not use the best tool for the task?
Shouldn't all views be using enums instead of isFirstRun?
It depends on how it's being used in the view, but if it's in the similar vein as we're doing here, then yeah, IMO it would make the view's code clearer, easier to understand, and nicer; however, given the time it would take to do this across the client, I don't think retrofitting it across the board is a good use of time.
We talked in the past about documenting our code standards. I believe we should finish that before we start adding to the standards. That way at least there's a reference for new coders and reviewers as well as forcing new code standards to be documented which would hopefully encourage a discussion first.
To be clear, I'm not trying to force any new standard on anyone. I have no issue going back and adding a comment for context. The thing I don't agree with is shoehorning new code into a style that predates the new capabilities that the move to BrighterScript gave us. If we can mitigate future maintainability issues and not paint ourselves into a corner now, I think we should.
| @@ -56,6 +108,10 @@ sub itemContentChanged() | |||
| itemData = item.json | |||
| m.top.id = itemData.id | |||
| m.top.findNode("moviePoster").uri = m.top.itemContent.posterURL | |||
| setWatchedColor() | |||
|
|
|||
| ' We dont' need to update everything if this is a reload | |||
There was a problem hiding this comment.
Why not update everything? It's possible the metadata changed especially if the refresh was triggered by a screensaver exit
There was a problem hiding this comment.
The goal of this PR is to update the resume data and the items associated with it, such as the poster image % bar. By only updating what is needed to meet that goal, we update the view as quickly and easily as possible to keep the loading status to a minimum.
Additionally, I've never heard anyone complain that while the screensaver is running that the metadata was changed on the server and when they exited the screensaver the data wasn't up to date. Is this really a problem scenario we need to address?
There was a problem hiding this comment.
So you don't want to update everything because it takes more time. Was that a problem in your testing?
Additionally, I've never heard anyone complain that while the screensaver is running that the metadata was changed on the server and when they exited the screensaver the data wasn't up to date. Is this really a problem scenario we need to address?
You want to leave a bug just because no one has noticed and complained about it? I really don't understand. Adding this line will create a bug and it's not needed to accomplish the stated PR goals.
In general, our users expect the data on the screen to be accurate regardless of how they got there. That's true of the home page. I used the screensaver as the example because it's possible for the screensaver to be up for hours or even days. Are you arguing that in those situations you think people deserve to have their data out of sync?
There was a problem hiding this comment.
So you don't want to update everything because it takes more time. Was that a problem in your testing?
Correct. The rule not only on Roku, but for everything, is to only update what you need to update. My head goes to React on the web, where one of its big selling points is only updating the smallest number of elements. I think this is the right mindset to keep.
Was that a problem in your testing?
Yeah, it was noticeable when I was working on the TV episodes. Originally it was reloading everything and the full update was taking over a second. By reworking it to only update the parts I wanted to target, it was refreshing noticeably faster. View redraws on Roku are not quick, so the more we isolate them, the better UX.
You want to leave a bug just because no one has noticed and complained about it?
I don't see this as a bug.
In general, our users expect the data on the screen to be accurate regardless of how they got there. That's true of the home page. I used the screensaver as the example because it's possible for the screensaver to be up for hours or even days. Are you arguing that in those situations you think people deserve to have their data out of sync?
Not even the web client does that level of data syncing. If I open a tab to a movie, open a 2nd tab to the same movie, then favorite the movie in 1 tab the other one isn't automatically updated to show the favorite icon. I'm not convinced people are expecting all data to dynamically refresh at all times.
There was a problem hiding this comment.
Correct. The rule not only on Roku, but for everything, is to only update what you need to update.
That's not what you're doing here though. Instead of updating what NEEDS to be updated(the data that has changed since last view), you're just hardcoding what data to update. It would take just as much processing to determine what NEEDS to be updated (because we don't have a cool framework like React) so why not just update everything? Yes it will take as long as the initial load but I don't see the problem there either. Why would a user expect a page to load faster the second time they use it?
Yeah, it was noticeable when I was working on the TV episodes. Originally it was reloading everything and the full update was taking over a second. By reworking it to only update the parts I wanted to target, it was refreshing noticeably faster. View redraws on Roku are not quick, so the more we isolate them, the better UX.
So why make the change on the movie detail screen if the problem only existed on the tv show view? IIRC the TV season view takes a long time to load because it polls the API for "extras" episodes. If the load time is such a problem on refresh, shouldn't we be addressing it on first load too?
How do you know users would prefer a shorter load time instead of having all their data be accurate? Our users haven't complained about the first load time but you're assuming they will complain about it taking the same amount of time on refresh? I don't agree and would personally prefer the data to be as accurate as possible.
I don't see this as a bug.
How do you expect our users to know what data is in sync and what data isn't? I consider it a bug and I can write up a ticket if that helps. "When refreshing the movie detail screen, some data is updated and some data isn't". As I mentioned, the general expectation is that the data on the screen is accurate just like with our home page.
Not even the web client does that level of data syncing. If I open a tab to a movie, open a 2nd tab to the same movie, then favorite the movie in 1 tab the other one isn't automatically updated to show the favorite icon. I'm not convinced people are expecting all data to dynamically refresh at all times.
Sounds like the web client has room for improvement. I don't see how that's relevant to what's best for our app/users. It's also unfair to compare every aspect of a TV based app to a web based app. Web client users can tap F5 or click refresh for example.
| @@ -56,6 +108,10 @@ sub itemContentChanged() | |||
| itemData = item.json | |||
| m.top.id = itemData.id | |||
| m.top.findNode("moviePoster").uri = m.top.itemContent.posterURL | |||
| setWatchedColor() | |||
There was a problem hiding this comment.
Shouldn't we be moving up some other code above the return statement as well? Like setFavoriteColor() m.buttonGrp.visible = true and stopLoadingSpinner()?
There was a problem hiding this comment.
Some of this answer is the same as above. The favorite color doesn't pertain to the resume state, so it's not included in this PR.
The stopLoadingSpinner() is called in main once all the items are completed - we have 3 things in total we need to do before we hide the spinner, and only 2 of them are handled in this function.
There was a problem hiding this comment.
Oh, and m.buttonGrp will already be visible by this point, because the view has already gone through this function the first time it was loaded and we aren't hiding m.buttonGrp again.
There was a problem hiding this comment.
The favorite color doesn't pertain to the resume state, so it's not included in this PR.
Technically setWatchedColor() doesn't pertain to the resume state either. You moved it up so the GUI would be updated and to prevent a bug. It's the same thing for setFavoriteColor(). It doesn't need to moved up but by leaving it where it is, a new bug will be created.
There was a problem hiding this comment.
Technically setWatchedColor() doesn't pertain to the resume state either.
I disagree. I think there is a direct line one could draw around resume state and watched state. The goal is to update the resume state, which I believe watched state is grouped with in this instance. Just like how we update the watched checkmark.
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks! |
|
So, what's our path forward on this one? It feels like cewert and I are currently at an impasse on both the code and refresh approach. |
|
To make the most informed comment, I would need to test this PR and really dig in to the code. As it stands from a quick overview, it looks like the update is written, and might only need some possible comments as it is. As it is an enhancement, would it not be possible to merge this and then discuss code standards later? If it turns out there is more to address, it could continue to be enhanced at a later point. There have been a lot of changes across the client apps and client app teams to enhance the speed/velocity of releases, we shouldn't block that further. |
|
Sorry for lack of response. Feel free to dismiss my review if needed.
This is tagged as bugfix which means after it's merged it will be in prod for the next bugfix release. I think that's where the disconnect is coming from because I was reviewing this PR as if it were being pushed to prod after merging. We wouldn't be able to enhance it until the next minor release. To recap my thoughts:
|
|
I too see this as a bug fix, not as an enhancement. |
|
My apologies, I was using "enhancement" in the general sense to mean "improvement", and not in the context of any programming or project management methodology. |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks! |
|
Closing in favor of #1771 |
Changes
Issues
Fixes #1647