Re: Re: CvsAlert changes

Am 10.12.2003 um 23:55 schrieb Jerzy Kaczorowski:

> Jens,
>
>> Just one modification I'd suggest: make CvsAlert::ShowAlert a const
>> member function. That would allow streamlined code like e.g.
>>      CvsAlert("Some message").ShowAlert();
>> for basic error messages.
>
> Why do we need consts?
> That works for me:
> CvsAlert(kCvsAlertNoteIcon, "Some message").ShowAlert();

Yeah, I guess you're right - don't know why I thought it had to be 
const :-|

>
> Unless you mean to skip the icon parameter, in which case we could 
> have an
> overloaded constructor. But I have the feeling that it's good to be 
> forced
> to specify the message type (icon) as it makes you think what you 
> doing ;)

No, that was just me being lazy when typing the example ;-)

>
>> And another thing I noticed: there's a const std::string m_empty
>> member in the CvsAlert class, but IMO, this could be a static
>> member and shared between all instances of CvsAlert?
>
> Actually I decided to remove it all together and return the copies of
> members instead of const references. It's the alert dialog after all 
> and
> there is no reason to optimize it. It's better to keep it simple ;)

Of course! And even cleaner that way...

>
>> I saw that e.g. for the alert asking to confirm moving files to
>> the trash, you specified BUTTONTITLE_YES and
>> BUTTONTITLE_NO as the button titles.
>> However, on the Mac, questions of this kind (i.e. confirming
>> operations) are not asked with "Yes" and "No" buttons, but rather with
>> "OK" and "Cancel" buttons. (Of course this requires that the wording 
>> of
>> the question itself is such that it's clear what OK and Cancel
>> means...)
>
> We have to go about it on a case-by-case basis. In the case of erasing 
> files
> in CvsCmdMoveToTrash the wording seems to match the Yes/No buttons 
> rather
> than OK/Cancel. On top of that the standard "Move to recycle bin" on 
> Windows
> uses the Yes/No buttons. But if you feel we have to customize that or 
> any
> other part then feel free to wrap the place with #ifdef's as needed. I 
> think
> it's better to have an external conditional compilation instead of the
> abstract concept of default buttons. It's simpler to avoid too many
> indirection. Eventually you could use different defaults in CvsAlert
> contructor but it has the OK/Cancel as defaults already.

O.k., I've conditionalized the code to set up the button titles in 
MoveToTrash code. (Confirmation dialogs should just never ask with 
"Yes" / "No" on the Mac according to the UI guidelines)
So far, this seems to be the only place that has been adjusted, but 
please remember when you change stuff for WinCvs that MacCvs doesn't 
really like the "Yes"/"No" buttons ;-)
(The only place where it asks for "Yes" / "No" is in the CvsYesNo 
dialog that asks the question from cvs)

>
>> My suggestion now is to allow CvsAlert to be called asynchronously by
>> implementing a member ShowAlertAsync where a completion callback can 
>> be
>> passed (preferably a functor) which will be called with the alert
>> result when the alert is dismissed:
>
> I think it's easier to make the CvsAlert::ShowAlert virtual and derive 
> a
> class from CvsAlert (like CvsAlertAsync) and then extend the derived 
> class
> constructor to pass the callback function and override ShowAlert to 
> handle
> asynchronous behaviour.
>
> I would not like to have too many functions to show the alert - it may 
> get
> very confusing with time ;)

Yes, why not... I just wanted to make sure we keep that option open now 
that we're redesigning CvsAlert.

I'll update to your latest changes later today and will probably commit 
the modifications needed for MacCvs tonight.

</jum>


[Non-text portions of this message have been removed]


------------------------ Yahoo! Groups Sponsor ---------------------~-->
Buy Ink Cartridges or Refill Kits for your HP, Epson, Canon or Lexmark
Printer at MyInks.com. Free s/h on orders $50 or more to the US & Canada.
http://www.c1tracking.com/l.asp?cid=5511
http://us.click.yahoo.com/mOAaAA/3exGAA/qnsNAA/NhFolB/TM
---------------------------------------------------------------------~->

To unsubscribe from this group, send an email to:
[email protected]

 

Your use of Yahoo! Groups is subject to http://docs.yahoo.com/info/terms/