37922 - Address book Buttons should be context dependent

Status: RESOLVED INVALID

(Thunderbird :: Address Book, defect)

Description

[Henrik Gemal]

The buttons in the toolbar of the addressbook should be context dependend. 
Meaning that:
the "New Msg" should only be avaiable when you highlight a entry.
the "Edit" should only be avaiable when you highlight a entry.

Comment 1

[lchiang]

I think nbaca's working on this (checking) for menus as well.

Comment 2

[scottputterman]

nbaca is this a dup?

Comment 3

[scottputterman]

moving to future milestone.

Comment 4

[scottputterman]

reassigning to chuang.

Comment 5

[Henrik Gemal]

This is still an issue.
Just delete an addressbook and the focus is nowhere.

Comment 6

[scottputterman]

reassigning to racham

Comment 7

[Henrik Gemal]

is there another bug about this or is this the "original" bug?

Comment 8

[Henrik Gemal]

*** Bug 67924 has been marked as a duplicate of this bug. ***

Comment 9

[Olga]

I changed QA contact from nbaca to olgam.

Comment 10

[Ninoschka Baca]

I think the New Msg button should be enabled at all times. I don't think it
should require an entry to be highlighted. I agree that the Edit menu should be
enabled to an entry is highlighted.

Comment 11

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 12

[Mark Banner (:standard8)]

This is my take on what we should do. On the address book toolbar we have the
following buttons, which are enabled as follows (currently):

New Card - Enabled all the time (note if LDAP is selected defaults to add to PAB)
New List - Enabled all the time (ditto)
Properties - Enabled all the time (i think this is a bug in itself though)
Compose - Enabled all the time  (no card or blank email = blank addressing widget)
IM - Enabled all the time
Delete - Enabled only when a deletable card/list/address book is selected.

I think these ones should be changed as follows, leaving the rest the same:

Properties - Enabled only when one card/list/address book is selected (intended
functionality I believe)
IM - Enabled only when the selected card has a screen name attached to it.

The edit menu will always be enabled, but the relevant items will be disabled
appropriately.

Comments?
If not I'll come up with a patch in the next couple of days.

Comment 13

[Mark Banner (:standard8)]

I've not got time to work on this at the moment

Comment 14

[Paul Tomlin]

I'll hook this up once bug 358333 lands

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #12)
> This is my take on what we should do. On the address book toolbar we have the
> following buttons, which are enabled as follows (currently):

Ok, slight revision (due to bug 358333 and further thoughts. This is what I think we should now do:

New Card - disabled if AB is readonly, otherwise enabled.
New List - as per bug 358333
Properties - Enabled only when one card/list/address book is selected (note if we fix the AB card not to be modal bug, then this may be when more than one card is selected)
Compose - Enabled all the time (no card or blank email = blank addressing widget)
IM - Enabled only when the selected card has a screen name attached to it
Delete - Enabled only when a deletable card/list/address book is selected.

Comment 16

[Paul Tomlin]

Attachment: as per comment 15

Seems to solve all the issues as per comment 15. 

Might be slightly intrusive on the goEdit[Card|List]Dialog side of things. The card version was needed to update cmd_newim, and I thought consistency was probably better. The only callers were changed to suit.

Solves bug 132435 and bug 257369 for added juicy-ness, and checked for nitty-ness of m/^\+.*\t/ and m/^\+.*\s+$/

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 246262 [details] [diff] [review]
as per comment 15

+  //FIXME: for some reason, mailnews/ gets CommandUpdate_AddressBook invoked
+  //       when the results pane selection changes, while mail/ doesn't. I
+  //       can't figure out why yet, so fix me. @ptomli for bug 37922
+  //       I think it's something to do with not using resultsPaneOverlay
+  CommandUpdate_AddressBook();
+

This could be because of the differences between these:

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abResultsPaneOverlay.xul#53
http://lxr.mozilla.org/seamonkey/source/mail/components/addrbook/content/addressbook.xul#618

+function onEditCardDialogAccepted()
+{
+  goUpdateCommand('cmd_newim');
+  UpdateCardView();
+}

Why not just do it in UpdateCardView? I know that's probably not quite as ideal - Ideally we'd have a nice callback from the nsIAbView telling us something had changed. Unfortunately it doesn't at the moment :-(

Comment 18

[Paul Tomlin]

(In reply to comment #17)
> This could be because of the differences between these:
> http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abResultsPaneOverlay.xul#53
> http://lxr.mozilla.org/seamonkey/source/mail/components/addrbook/content/addressbook.xul#618
> 

Yes, hence my note below my initial confusion. I left the FIXME in the hopes you'd enlighten me on the preferred practice when meeting this kind of juncture.

I can either:
i. mimic the mailnews/ version by firing the same event in mail/
ii. accept the differences and remove the FIXME

> Why not just do it in UpdateCardView? I know that's probably not quite as ideal

UpdateCardView has a happy, clean and well defined task to do. Updating cmd_newim doesn't really have anything to do with refreshing the details pane of a card, except they sometimes occur as a result of the same event[s].

I don't think the calling overhead is significant enough here to warrant some premature optimisation.

The other part of me is not wanting an included file (abCommon) to be invoking functions defined in the including file (addressbook, etc), but I think that change is outside the scope of this bug.

Comment 19

[Mark Banner (:standard8)]

(In reply to comment #18)
> (In reply to comment #17)
> > This could be because of the differences between these:
> > http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abResultsPaneOverlay.xul#53
> > http://lxr.mozilla.org/seamonkey/source/mail/components/addrbook/content/addressbook.xul#618
> > 
> 
> Yes, hence my note below my initial confusion. I left the FIXME in the hopes
> you'd enlighten me on the preferred practice when meeting this kind of
> juncture.
> 
> I can either:
> i. mimic the mailnews/ version by firing the same event in mail/
> ii. accept the differences and remove the FIXME

I think do the first. It may also explain why TB address book starts up with all buttons (apart from compose) disabled, whereas SM comes up with PAB selected and most buttons enabled (I prefer the SM way).
> 
> > Why not just do it in UpdateCardView? I know that's probably not quite as ideal
> 
> UpdateCardView has a happy, clean and well defined task to do. Updating
> cmd_newim doesn't really have anything to do with refreshing the details pane
> of a card, except they sometimes occur as a result of the same event[s].
> 
> I don't think the calling overhead is significant enough here to warrant some
> premature optimisation.

Ok, that's fine.

Another strange effect with this patch:
1) With AB or card selected, note appearance of buttons.
2) Now do a quick search.
3) Clear quick search

Result: All buttons disabled.
Expected: I think we should still allow New Card & List, but disable properties etc as the focus is on the quick search box (or clear button) but the address book is still selected.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 246262 [details] [diff] [review]
as per comment 15

see comment 19

Comment 21

[Wayne Mery (:wsmwk)]

seems awfully close.

Paul, are you still up for this?

Comment 22

[Tony Mechelynck [:tonymec]]

I can still reproduce in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090613 SeaMonkey/2.0b1pre - Build ID: 20090613000642

Reassigning to default for lack of an answer to comment #21

Comment 23

[Wayne Mery (:wsmwk)]

Design is different in version 102