Closed Bug 132453 Opened 19 years ago Closed 6 years ago

Multi select cards and the Properties button should be disabled

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: nbaca, Assigned: aceman)

References

(Depends on 1 open bug)

Details

(Keywords: polish, Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

Trunk build 2002-03-20: WinMe, Linux RH 7.1, Mac 10.1.3

Overview: If bug# 132446 is not fixed then the Properties button should be
disabled when multilple cards are selected.
.
Assignee: racham → sspitzer
Summary: Multi select cards and the Properties button should be disabled → Multi select cards and the Properties button should be disabled
Whiteboard: [good first bug]
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Adding dependency to bug 37922 as I'll probably be fixing this under that bug.
Depends on: 37922
Assignee: mail → nobody
Component: Address Book → MailNews: Address Book
OS: Windows ME → All
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Pulling in 37922 and deps onto my task list
Assignee: nobody → ptomli.bugzilla
Product: Core → MailNews Core
Assignee: ptomli.bugzilla → nobody
Aceman, could you pls test this patch if it fixes the issue?
Attachment #8425378 - Flags: feedback?(acelists)
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Keywords: polish
Comment on attachment 8425378 [details] [diff] [review]
Patch V.1: Disable Properties button for multiple selections

Review of attachment 8425378 [details] [diff] [review]:
-----------------------------------------------------------------

You do not need the empty Date, Node ID, Parent lines. But add '# ' before 'HG changeset patch'.

The file is in mailnews/addrbook/content/abResultsPane.js, not mailnews/addrbook/resources/content/abResultsPane.js .
But I don't know in which dialog I should try to fix this. We have Properties in Find->Addresses (this works fine without the patch), in Addressbook, in Contacts sidebar in composer. But I didn't notice this would fix it anywhere.

As far as I could debug, your line was never executed in my tests.

I am playing with it a bit and it seems much more involved. But it seems I could do something with it if you wish.
Attachment #8425378 - Flags: feedback?(acelists) → feedback-
Attached patch patch v2 (obsolete) — Splinter Review
mconley, can you please check if really this much is needed? I just made the edit/properties buttons/menuitems observe the cmd_properties command. I didn't use the double personality that Delete uses (there are both cmd_delete and button_delete). Not sure what that is for. The /mailnews code now knows both cmd_properties and button_edit because SM uses button_edit.

For me it works now in the addressbook window and also compose contacts sidebar.
Attachment #8425689 - Flags: feedback?(mconley)
Thanks aceman for taking over, which will definitely speed this up in better hands ;) ... I was suspecting something along the lines of your patch because the old code which I swapped out didn't look wrong as such... So I'm glad that my tentative try patch has sparked this off, perhaps I should try that trick more often ;)
Assignee: bugzilla2007 → acelists
Comment on attachment 8425689 [details] [diff] [review]
patch v2

Review of attachment 8425689 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks like the right idea.
Attachment #8425689 - Flags: feedback?(mconley) → feedback+
Guys, would you like this in SM too? Do you see a need for both cmd_properties and button_edit or can we unify it?
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)
Comment on attachment 8425689 [details] [diff] [review]
patch v2

>+function CommandUpdate_Addressbook()
...
>+              oncommandupdate="CommandUpdate_AddressBook()">
Oops?

>-        return (GetSelectedCardIndex() != -1);
>+        // While "Edit Contact" dialogue is still modal (bug 115904, bug 135126),
>+        // only enable "Properties" button for single selection; then fix bug 119999.
>+        return (GetNumSelectedCards() == 1);
(As it happens GetSelectedCardIndex returns -1 unless the number of selected cards is 1)
(In reply to aceman from comment #9)
> Guys, would you like this in SM too? Do you see a need for both
> cmd_properties and button_edit or can we unify it?

I don't see any reason not to rename button_edit to cmd_properties.

(I don't know why the delete button is the one element that doesn't observe button_delete...)
Flags: needinfo?(neil)
I have nothing to add over what Neil said.
Flags: needinfo?(iann_bugzilla)
(In reply to neil@parkwaycc.co.uk from comment #10)
> >+function CommandUpdate_Addressbook()
> ...
> >+              oncommandupdate="CommandUpdate_AddressBook()">
> Oops?
> 
What? I copied this, but "oncommandupdate" is a real attribute. Or do you mean anything else?
Attached patch patch v3 (obsolete) — Splinter Review
So this is a polished version, also with Seamonkey support (but untested).
I have seen just one small glitch: when contacts sidebar in compose window is opened and the first card is selected, rightclick on it shows Delete and Properties disabled. Since then, the update of the commands seems to work fine. I don't yet know what's up with that. The goDocommand("cmd_properties") IS executed even at the first click.
Attachment #8425689 - Attachment is obsolete: true
Attachment #8437894 - Flags: review?(neil)
Attachment #8437894 - Flags: review?(mconley)
(In reply to aceman from comment #13)
> (In reply to comment #10)
> > >+function CommandUpdate_Addressbook()
> > ...
> > >+              oncommandupdate="CommandUpdate_AddressBook()">
> > Oops?
> > 
> What? I copied this, but "oncommandupdate" is a real attribute. Or do you
> mean anything else?

The captialisation.
(In reply to neil@parkwaycc.co.uk from comment #15)
> (In reply to aceman from comment #13)
> > (In reply to comment #10)
> > > >+function CommandUpdate_Addressbook()
> > > ...
> > > >+              oncommandupdate="CommandUpdate_AddressBook()">
> > > Oops?
> > > 
> > What? I copied this, but "oncommandupdate" is a real attribute. Or do you
> > mean anything else?
> 
> The captialisation.

Oh, thanks. It is messed up in more places.
I wonder why it didn't throw or something...
Attached patch patch v3.1Splinter Review
Attachment #8425378 - Attachment is obsolete: true
Attachment #8437894 - Attachment is obsolete: true
Attachment #8437894 - Flags: review?(neil)
Attachment #8437894 - Flags: review?(mconley)
Attachment #8438621 - Flags: review?(neil)
Attachment #8438621 - Flags: review?(mconley)
Comment on attachment 8438621 [details] [diff] [review]
patch v3.1

r=me on the button_edit/cmd_properties rename.
Attachment #8438621 - Flags: review?(neil) → review+
Comment on attachment 8438621 [details] [diff] [review]
patch v3.1

Review of attachment 8438621 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this. Thanks aceman!
Attachment #8438621 - Flags: review?(mconley) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/22b623fefdd3 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Keywords: checkin-needed
Blocks: 1133355
You need to log in before you can comment on or make changes to this bug.