Re: [comctl32] MonthCalendar - Fix wrong date selected when clicknext/prev months day (#6967)

[Dmitry Timoshkov]

"Vijay Kiran Kamuju"  wrote:
> - retval = *daypos + (7 * *weekpos) - firstDay;
> - retval = *daypos + ((7 * *weekpos) - firstDay);
> return retval;
>
> }

This change doesn't make any difference, neither in functionality,
nor in readability IMO. Please avoid this kind of changes, it only
clutters the patch, making it hard to see what is the real change.
> - lpht->st.wDay   = MONTHCAL_MonthLength(lpht->st.wMonth,lpht->st.wYear) -day;
> - lpht->st.wDay   = day+MONTHCAL_MonthLength(lpht->st.wMonth,lpht->st.wYear) ;

A couple of spaces around '+' won't hurt, and a space before ';' doesn't match
an existing code style.
> - return TRUE;
> - /*return TRUE;*/

If you are not sure that the return should be removed, it's a sign that you
need a test case, just commenting it out without any explanation is not a solution.
Same comment to all other similar cases.
> - MONTHCAL_CopyTime(&infoPtr->minSel,&nmsc.stSelStart);
> - MONTHCAL_CopyTime(&infoPtr->maxSel,&nmsc.stSelEnd);
> nmsc.nmhdr.hwndFrom = infoPtr->hwndSelf;
> nmsc.nmhdr.idFrom   = GetWindowLongPtrW(infoPtr->hwndSelf, GWLP_ID);
> nmsc.nmhdr.code     = MCN_SELCHANGE;
> - MONTHCAL_CopyTime(&infoPtr->minSel,&nmsc.stSelStart);
> - MONTHCAL_CopyTime(&infoPtr->maxSel,&nmsc.stSelEnd);

This change is not needed.

[comctl32] MonthCalendar - Fix wrong date selected when clicknext/prev months day (#6967)

[Vijay Kiran Kamuju]

Hi,

On 12/25/06, Dmitry Timoshkov  wrote:
> "Vijay Kiran Kamuju"  wrote:
> > - retval = *daypos + (7 * *weekpos) - firstDay;
> > - retval = *daypos + ((7 * *weekpos) - firstDay);
> > return retval;
> >
> > }
>
> This change doesn't make any difference, neither in functionality,
> nor in readability IMO. Please avoid this kind of changes, it only
> clutters the patch, making it hard to see what is the real change.

This was a mistake I didnt notice.
> > - lpht->st.wDay   = MONTHCAL_MonthLength(lpht->st.wMonth,lpht->st.wYear) -day;
> > - lpht->st.wDay   = day+MONTHCAL_MonthLength(lpht->st.wMonth,lpht->st.wYear) ;
>
> A couple of spaces around '+' won't hurt, and a space before ';' doesn't match
> an existing code style.

Will change this one, I was bit out of style.
> > - return TRUE;
> > - /*return TRUE;*/
>
> If you are not sure that the return should be removed, it's a sign that you
> need a test case, just commenting it out without any explanation is not a solution.
> Same comment to all other similar cases.

Should I add comments or Do I have to add test cases?
test cases are bit difficult as they are all UI based, and dont have
exact testcases.
As I rearranged the codes and removed the returns as the necessary
messages are not being sent (in order as observed on windows using
both old and latest versions of ControlSpy)
If you think I should add comments, I would be glad to modify.
Tests are a bit difficult :( (if you can help, I will try)
> > - MONTHCAL_CopyTime(&infoPtr->minSel,&nmsc.stSelStart);
> > - MONTHCAL_CopyTime(&infoPtr->maxSel,&nmsc.stSelEnd);
> > nmsc.nmhdr.hwndFrom = infoPtr->hwndSelf;
> > nmsc.nmhdr.idFrom   = GetWindowLongPtrW(infoPtr->hwndSelf, GWLP_ID);
> > nmsc.nmhdr.code     = MCN_SELCHANGE;
> > - MONTHCAL_CopyTime(&infoPtr->minSel,&nmsc.stSelStart);
> > - MONTHCAL_CopyTime(&infoPtr->maxSel,&nmsc.stSelEnd);
>
> This change is not needed.

I agree. This came up because it while doing regression testing with
new control spy V2.0 where MULTISELECT message is not eanbled by
default.

Should I send a new version of the patch, with the changes?
Waiting for your reply.
---
VJ

[comctl32] MonthCalendar - Fix wrong date selected whenclicknext/prev months day (#6967)

[Dmitry Timoshkov]

"Vijay Kiran Kamuju"  wrote:
> > > - return TRUE;
> > > - /*return TRUE;*/
> >
> > If you are not sure that the return should be removed, it's a sign that you
> > need a test case, just commenting it out without any explanation is not a solution.
> > Same comment to all other similar cases.
>
> Should I add comments or Do I have to add test cases?

That's up to you, but having any code simply commented out without
a single word of an explanation is confusing and misleading. I'd say
that having a commented out 'return' with any sort of an explanation
is bad IMO.
> test cases are bit difficult as they are all UI based, and dont have
> exact testcases.
> As I rearranged the codes and removed the returns as the necessary
> messages are not being sent (in order as observed on windows using
> both old and latest versions of ControlSpy)
> If you think I should add comments, I would be glad to modify.
> Tests are a bit difficult :( (if you can help, I will try)

Of course the tests are preferable, but if it's a pure UI thing it's
pretty hard to test, yes. Message sequences can be tested, but that's
a bit trickier since comctl32 doesn't have a message sequnce test
infrastructure.
> Should I send a new version of the patch, with the changes?

I don't think that the patch will be magically fixed on its own.