[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kmail-devel
Subject:    Re: [PATCH] - For bug 76256 - Load external references on demand
From:       Ingo =?windows-1252?q?Kl=F6cker?= <kloecker () kde ! org>
Date:       2004-04-19 22:01:41
Message-ID: 200404200001.46839 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Monday 19 April 2004 23:01, Jason Keirstead wrote:
> I've created a patch to allow one to load external references on
> demand, as requested by bug 76256. I was wondering if soemone could
> review it before I commit it, I have never put code into KMail
> before.
>
> It is actually very straight-forward.
>
> See http://bugs.kde.org/show_bug.cgi?id=76256 for the patch. Thanks.

I'm a lazy guy, so please always send patches to the mailing list 
instead of or additionally to attaching them to a bug report. It's so 
much easier to review an attached patch in KMail and to save an 
attachment from KMail than to go to the bug report, search the 
attachment and click on it to view/save it.

If you want your patch to get our attention then you'll anyway have to 
send a note to kmail-devel. So attaching patches to bug reports doesn't 
really make much sense (unless it's a patch which can't be applied 
immediately, e.g. during feature freeze).

Some comments on your patch:
- Your patch still deletes a line which must not be deleted.
- Don't change the version of kmreadermainwin.rc unless it's necessary.
- You can't bind L to "Load External References" because this is already 
used by Reply to Mailing-List. Actually, please don't bind any key to 
the Load External References action by default.
- Using the accelerator L as in "&Load External References" is not 
possible because L is already taken by Reply to Mailing-&List.
- Please rename mLocalRefs to mLocalRefsOnly. The latter makes the 
negation !mLocalRefsOnly much easier to understand.
- Please use 2 spaces for indentation, but not 4 as in 
KMReaderWin::slotLoadExternalReferences().
- The following doesn't work correctly:
+      loadExternalReferencesAction()->setEnabled( single_actions );
  Reason: This action should only be enabled if viewing HTML messages is 
enabled as in the following line
+    mExternalReferencesAction->setEnabled( mHtmlMail );

Send the updated patch to this mailing list.

Regards,
Ingo

[Attachment #5 (application/pgp-signature)]

_______________________________________________
KMail developers mailing list
KMail-devel@kde.org
https://mail.kde.org/mailman/listinfo/kmail-devel


[prev in list] [next in list] [prev in thread] [next in thread]