calendar items now emit an item changed signal whenever it's changed in someway#80
calendar items now emit an item changed signal whenever it's changed in someway#80TimGroeneboom wants to merge 3 commits intomainfrom
Conversation
…d in someway - comparison operators added to both Date and Time structs for readable comparison
|
Changed target to 0.8 - 0.7.X is locked except for bug fixes |
cklosters
left a comment
cklosters
left a comment
There was a problem hiding this comment.
Although I welcome the change there are some obvious and avoidable issues that hould shouldn't be there
| Time backup = mPoint.mTime; | ||
| mPoint.mTime = time; | ||
|
|
||
| if (!mPoint.valid()) |
There was a problem hiding this comment.
You check the currently stored value for validity, not the new one, which is wrong. Should be:
// Apply and validate
auto cache = mPoint.mTime;
mPoint.mTime = time;
if (mPoint.valid())
{
if (mPoint.mTime != cache)
changed(*this);
return true;
}
// Restore if not valid
mPoint.mTime = cache;
return false;
| bool WeeklyCalendarItem::setDay(EDay day) | ||
| { | ||
| if (day != EDay::Unknown) | ||
| if (day != EDay::Unknown && day != mDay) |
There was a problem hiding this comment.
This returns false when the day is not different from the current one, which is wrong. Should be:
if (day != EDay::Unknown)
{
if (day != mDay)
{
mDay = day;
changed(*this);
}
return true;
}
return false;
| if (!date.valid()) { return false; } | ||
| mDate = date; | ||
| if (date != mDate) | ||
| changed(*this); |
There was a problem hiding this comment.
This will never trigger changed because you compare the same (now stored) value? Should be:
if (date.valid())
{
if(mDate != date)
{
mDate = date;
changed(*this);
}
return true;
}
return false;
| mDay = day; | ||
| if (mDay!= day) | ||
| changed(*this); | ||
| return true; |
There was a problem hiding this comment.
Same as above, changed will never be triggered
| mDay = day; mMonth = month; | ||
| if (mDay != day || mMonth != month) | ||
| changed(*this); | ||
| return false; |
There was a problem hiding this comment.
Same as above, changed will never be triggered
| // Comparison operators | ||
| bool operator == (const Time &c) const | ||
| { | ||
| if (mHour == c.mHour && mMinute == c.mMinute) |
There was a problem hiding this comment.
simplify -> return c.mHour == mHour...
| bool valid() const; ///< Returns if time is valid | ||
|
|
||
| // Comparison operators | ||
| bool operator == (const Point &c) const |
There was a problem hiding this comment.
simplify -> return c.mTime == mTime ...
|
I fixed the above issues here: aecd546, feel free to merge the calendar_itemchanged_fixes branch into this one if you accept my changes. |
|
Closing this due to inactivity. Feel free to create a new PR after you validate my |
2 participants
The following PR solves a long standing feature that I wanted to implement. Often in installations I need to perform a certain action when a user changes something in the operation calendar, either in the app or using the web client. This way I can listen to an update of the operation calendar when the user changes the calendar. Most of the times, I just want to save the operational calendar instantly then.