Skip to content
This repository has been archived by the owner on Oct 9, 2020. It is now read-only.

fix: fixed bookmark bug in DayScheduleFragment - Fixes #2352 #2358

Closed
wants to merge 1 commit into from
Closed

fix: fixed bookmark bug in DayScheduleFragment - Fixes #2352 #2358

wants to merge 1 commit into from

Conversation

sriramr98
Copy link
Contributor

Fixes #2352
Changes: Removed loadSessions method from onCreateView and added it in onResume. Since there was no new loading of events after a bookmark was changed, loading new data in onResume fixed the problem.

@iamareebjamal
Copy link
Member

There are several unneeded changes and the method of adding it in onResume is a workaround and not a fix

@sriramr98
Copy link
Contributor Author

Those unneeded changes are just changes in space in the code and optimizations of imports. And why won't a workaround be accepted? If the workaround fixes the bug and has no other problems, isn't it a fix? I am sorry but I am new to open source and I just don't understand what the difference is.

@iamareebjamal
Copy link
Member

iamareebjamal commented Mar 8, 2018

Unneeded changes:

  • Commented code. Code which is commented should be removed, it is dead weight
  • Optimised Imports - The PR should have 1:1 correspondence with the issue. A 5 line fix may be masked by 100 line changes of reformatting, that's why we usually do formatting of only the code which is added or in a separate PR. It also makes it difficult to review and pollutes git blame, you are accounted for code you didn't write

The difference is that workarounds which have no other options other than to use them are OK, but with a huge warning and TODO tag that this is danger zone. Fix this ASAP. But, here we close the issue with your PR and no one bothers to look into what the real issue was which was not the fact that the code was not in onResume

2 months later, someone else writes this code and there occurs the same issue which was closed months ago and solution? Use the same workaround and forget that the issue exists.

On the other hand, if the issue is handled correctly the first time, not only we save ourselves from code smell in the future, but also from the bug ever appearing again

@dr0pdb
Copy link
Contributor

dr0pdb commented Mar 8, 2018

@sriramr98 Hi!

Those unneeded changes are just changes in space in the code and optimizations of imports.

Yes, but it often distracts the reviewers from the actual context of the PR and the issue. Once there are a lot of these types of formatting errors feel free to create an issue for it and remove it for the whole project.

If the workaround fixes the bug and has no other problems

Yes, if a workaround solves the bug and has no other problem then it's a fix IMO. But this one has a few problems.
Adding it in the onResume means it will be called more often which will lead to more operations which are unnecessary. For eg. The bookmarks will be reloaded everytime the screen is rotated which is unnecessary.

Also the aim of using LiveData and Reactive Programming in general is to make the application react to changes, not check for changes. So it should automatically update it when there are changes.

@iamareebjamal
Copy link
Member

@sriramr98 Status?

@sriramr98
Copy link
Contributor Author

I don't have a fix as of now. Will try to do something today if possible.

@sriramr98
Copy link
Contributor Author

@iamareebjamal I think this PR can be closed. I don't have a fix and I am not working on it as of now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events bookmarked on Schedule does not update the DayFragmentSchedule
3 participants