CMPUT301F21T23 / HabitTracker

2 stars 1 forks source link

Habits #118

Closed PaoVal closed 3 years ago

PaoVal commented 3 years ago

Alright buckle up, this is a big PR.

This PR deals with:

Things I modified duringx merge conflict:

NickSNFK commented 3 years ago

I added a couple of comments. I am not done reviewing yet. I was trying to test your code on AndroidStudio. The DatabaseManager is throwing errors because you changed the constructor of Habit.

This because there is a new field in the Habit Class (titleDisplay), can we get this updated in the database manager?

PaoVal commented 3 years ago

I added a couple of comments. I am not done reviewing yet. I was trying to test your code on AndroidStudio. The DatabaseManager is throwing errors because you changed the constructor of Habit.

I am assuming this is not an issue anymore @zarifmahfuz ?

PaoVal commented 3 years ago

@zarifmahfuz

When I click on “Edit”, the fragment fields are not pre-filled by the original details. This makes things harder for the user. We must address this in the future.

I was planning on doing this but ran out of time. Also, once again, having the same fragment for adding and editing really restricts what you can do here. I suggested changing this a while back to no avail. And now there is no time to do so. Maybe for the next deliverable

Since John is using the key of a habit to display habit titles on his habit events screen, the change made by editing the habit title does not get reflected on his screen. This is something we need to address in the future.

Tried to fix this but will take more than I thought. SO I suggest we push this back to the next deliverable.

zarifmahfuz commented 3 years ago

comments. I am n

Correct

NickSNFK commented 3 years ago

Would the changes in Habit.java class affect getAllHabits method in DatabaseManager class? I'm using the method to get all Habit titles. If not then it's good to merge.

I believe @PaoVal changed it already

PaoVal commented 3 years ago

Would the changes in Habit.java class affect getAllHabits method in DatabaseManager class? I'm using the method to get all Habit titles. If not then it's good to merge.

I made some changes there so we have getAllHabits separate from what I made.

I think we should only have one function for this in the future. Though it works as implemented right now.

PaoVal commented 3 years ago

I am going to merge this since everyone seems ok with it