fix: fixed bookmark bug in DayScheduleFragment - Fixes #2352

fossasia / open-event-droidgen

Open Event Android App Generator https://github.com/fossasia/open-event-android/raw/apk/sample-apk-fossasia17-development.apk

GNU General Public License v3.0

2.06k stars 828 forks source link

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

Closed sriramr98 closed 6 years ago

sriramr98 commented 6 years ago

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 commented 6 years ago

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

sriramr98 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@sriramr98 Status?

sriramr98 commented 6 years ago

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

sriramr98 commented 6 years ago

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