Closed Bug 691141 Opened 13 years ago Closed 6 years ago

Click OK does not close address book entries after editing contact, if picture has been removed or location has changed

Categories

(Thunderbird :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: alther, Assigned: samuel.mueller)

References

Details

(Whiteboard: [gs][to be applied on top of bug 892889])

Attachments

(5 files, 10 obsolete files)

47.59 KB, image/png
Details
89.09 KB, image/png
mconley
: feedback+
Details
27.78 KB, image/png
Details
2.08 KB, patch
aceman
: review+
Details | Diff | Splinter Review
34.87 KB, patch
aceman
: review+
aceman
: ui-review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a2) Gecko/20111001 Firefox/9.0a2
Build ID: 20111001042013

Steps to reproduce:

Using TB 7.0.1.  I attempted to update an existing address book entry.  I updated the contact's work number and their work information (org, address, city, state, zip and web page).




Actual results:

When I clicked "OK" to save the update nothing happened.  Cancel and "X" worked to close the dialog.

When I click cancel, I see the information has been updated in the top-right pane (where the contacts are displayed in detail in rows.).  However, the contact information listed in the bottom-right pane, the full information of the selected contact) does not show the updates.

If I edit the contact again, the information I entered is still there, but OK still does not work, and I must cancel/X out.

If I highlight another contact in the top-right pane and then go back to the contact I originally edited, the updated information shows in the bottom-right pane.

If I now click on the new web address I entered nothing happens (no page is opened in the browser).

Finally, if I close out the Address Book, then reopen it, all my updates to the contact are gone and the original information prior to my update is there.

Looking at abook.mab, the Date Modified does not show the current (or recent) timestamp.




Expected results:

The contact info should have been updated and saved to abook.mab.
Rick, please go to config editor (Tools | options | advanced | general | config editor) and paste ldap_2.servers into filter. attach what you see there as screen shots to this bug
As requested, my list of ldap_2.servers preferences.

I'll just note that the address book entries I was trying to add are not on the LDAP - they were local.
hum xref 609074.
Sounds like my problem.  I'll note that my abook.mab is not hidden or read-only.  I don't know when this problem actually started to occur (I just happened to want to update a contact the other day).  

The last modified date on my abook.mab file is Oct, 1, 2011.  Not sure if it was touched by the TB 7 upgrade as I don't recall editing a contact at that time.
Rick, have you assigned a photo from your computer to that contact? If yes, did you delete or rename the photo?
Yes, I did have a photo associated with it.  The picture shows up as a thumbnail on the "summary" view, but the picture does not appear on the "Photo" tab of the contact's card.

I haven't renamed, deleted or renamed the file.  However, it did changed locations when I upgraded from XP to Win 7.

I then pointed to the same file in the new location and I'm now able to save changes to the contact.

This is going to be quite a problem as people upgrade.

Clearly, TB needs to be able to handle the situation where the photo can no longer be found.  It would even be nice if on the Photo tab it could let the user know it can no longer find the photo and the user can take further action if they choose.

Thanks!
great detective work!
I can also reproduce. Nothing in error console
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Unable to save address book entries after editing them. → Click OK does not close address book entries after editing contact, if picture has been removed or location has changed
Whiteboard: [gs]
Wayne, it was not detective work. I stumbled upon this bug yesterday (#694678), then found this one because of the same symptoms.

I will take it, but at the same time I would work on two other issues mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=694678#c2 and bug 548266. what's the opinion of address book's responsible? who is this anyway?
Samuel:

Hey!  Thanks for volunteering to tackle these bugs!

I'm the guy you probably want to talk to regarding address book bugs.  I'm mconley in #maildev on irc.mozilla.org, if you'd like to chat - or you can send me mail.

-Mike
Assignee: nobody → samuel.mueller
Status: NEW → ASSIGNED
Blocks: 609074
Attached image implementation draft
the implementation draft addresses the following bugs/features
 * load photos from the profile cache instead from the original source
 * download/copy a file async instead of synchronous
 * display a progress meter and a label below the image showing the download progress and errors that might occur
 * verify that the file is actually an image
 * downsize a large image to safe disk space
Attachment #567890 - Flags: feedback?(mconley)
Comment on attachment 567890 [details]
implementation draft

This looks good to me.

I particularly like the use of nsIWebBrowserPersist for async file download.

Would you mind putting together a mock-up / wireframe to show me where you feel that the progress bar and cancel buttons should go?

But yes, I'm happy with this proposal.  Great work!
Attachment #567890 - Flags: feedback?(mconley) → feedback+
Attached image mockup of proposed ui changes (obsolete) —
while a download is going on, the photo is hidden to make space for the new elements.
Comment on attachment 568060 [details]
mockup of proposed ui changes

Tossing over to bwinton for ui-review / feedback.
Attachment #568060 - Flags: ui-review?(bwinton)
Comment on attachment 568060 [details]
mockup of proposed ui changes

Hey Samuel:

Ok, a few issues with this - 

1)  There's something a bit odd with having all that widgetry on the left "floating in space".  I'd prefer it to be in some kind of box, since my eyes are looking for lines to keep things orderly.

2)  If such a box exists, then the Cancel button should probably be lined up with the bottom of it.

3)  Why is there a progress bar and an error message at the same time?  I would argue that we should display one, or the other, never both.

4)  I would also propose that we use a spinner instead of a progress bar, since we cannot guarantee:

  a)  That we know the size of the file being downloaded
  b)  That we know how long until the photo is resized

5)  In your screenshot, we've hit an error.  What does the Cancel button do exactly?  Does it revert the progress bar / message / button to the old photo, and reset the photo input fields?

Thanks,

-Mike
Attachment #568060 - Flags: ui-review?(bwinton) → ui-review-
Attached image mockup of proposed ui changes (obsolete) —
Hi mike, thanks for your comments - here is my new proposal.

(In reply to Mike Conley (:mconley) from comment #14)
> 
> 1)  There's something a bit odd with having all that widgetry on the left
> "floating in space".  I'd prefer it to be in some kind of box, since my eyes
> are looking for lines to keep things orderly.
Agreed. I added a second <groupbox>

> 
> 2)  If such a box exists, then the Cancel button should probably be lined up
> with the bottom of it.
The cancel button exists no more. The action is cancelled either by selecting a new photo or by pressing the cancel button of the dialog.

> 
> 3)  Why is there a progress bar and an error message at the same time?  I
> would argue that we should display one, or the other, never both.
It was never intended to show both at the same time. They were just there for illustration purposes.

> 
> 4)  I would also propose that we use a spinner instead of a progress bar,
> since we cannot guarantee:
> 
>   a)  That we know the size of the file being downloaded
>   b)  That we know how long until the photo is resized
What do you mean by spinner? I'm still for a progress bar. Start mode=undetermined and switch to determined if we get the first progress change.

> 
> 5)  In your screenshot, we've hit an error.  What does the Cancel button do
> exactly?  Does it revert the progress bar / message / button to the old
> photo, and reset the photo input fields?
Cancel button is not needed - see comment above.
Attachment #568060 - Attachment is obsolete: true
(In reply to Mike Conley (:mconley) from comment #14)
> 5)  In your screenshot, we've hit an error.  What does the Cancel button do
> exactly?  Does it revert the progress bar / message / button to the old
> photo, and reset the photo input fields?
The input fields are never reverted and the photo is never reset. Because the photo is saved based on user action (and not when closing the dialog), the photo will not change if an error occurred. The message / progress bar are reset / hidden when either a photo is displayed or a new file transfer starts.
Hey Samuel:

I like your latest mock-up!  I really do think a spinner is more appropriate though - here's what I mean by spinner:  https://www.loopt.com/images/spinner.gif?1318890481

I'm sure we have a spinner as a resource somewhere in Thunderbird (or Gecko's) arsenal of graphics that you could use.

Other than that, I like this.

-Mike
There is another issue: since the photos will be loaded from the cache (Photos directory) and the web URI / local file are only stored for reference, how are other add-ons going to update the photo? 
Is it sufficient that other add-ons simply include abCommon.js and use the functions defined there?
If yes, those functions need an additional parameter which is passing the card, because the card needs to be updated. On a second thought, this is also required for the async operation... but is it possible in the current implementation of the edit-card-window?
Hello everyone, any update on this issue? I would really like to be able to use pictures in the adressbook... Otherwise I will change my mail client!
No longer blocks: 609074
OS: Windows 7 → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
Version: 7 → Trunk
Attached image new ui changes
I realized that hiding the currently assigned photo is not that good from a UX point of view. The current photo should always be visible, as it is still assigned if the download fails.
I propose to move the download/copy status inside the right groupbox.
Any feedback is appreciated!
Attachment #568831 - Attachment is obsolete: true
Depends on: 892889
This seems to fix the same problem as bug 548266 (which already has a patch by me).

But your mockups look nice, so if you can code all that then you can take bug 548266 too and also bug 748664 and bug 892889 so that they are closed in parallel.

Also make sure all this is not obsoleted by mconley's addressbook rewrite.
Yeah I finally startet working on that issue a couple of days ago after the concept has been lying around for months, in fact it's almost finished by now.

I do not seem to have the rights to assign myself to those bugs. How can I get that permission?

Is mconley still working on the address book rewrite? I thought that thunderbird development has been stalled last year? That's actually the reason why I did not work in this bug for such a long time. In any case mconley is informed as he is in the cc list.
You can judge from the commit log at http://hg.mozilla.org/comm-central/ whether TB development is stalled. The dates of the patches in the log do not represent date of commit. All those patches in the first page were landed today.

mconley definitely works on the addressbook and for now promises to finish it. I just don't know if it only rewrites the backend or also affects the UI much. So you need his advice if your work on the photos is worthwhile.

I can assign you the bugs if you need. You can also try contacting the user 'gerv' for bugzilla rights.
calls to application.console.log will be removed in the final version.
Attachment #775335 - Flags: review?(mconley)
Nice, what a big code change :) Does this already cover using the cache image in profile/Photos in subsequent edits, or is that to be done later?
Blocks: 548266
Hey Samuel - thanks so much for your work here.

I'll start to look at the code here, but looking at the screenshot, I'm still not 100% happy with the position of the progress bar.

I think I'd prefer it *below* the radio buttons that allow us to select the contact image. The eye moves down the list, and after a selection is made, it's strange for activity *above* the list. It should be *below* the list.
Comment on attachment 775335 [details] [diff] [review]
async download of contact photos. requires applied patch from bug bug 892889.

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

Good work in here Samuel. Just a few concerns, see below.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +925,5 @@
>   *
>   */
>  function loadPhoto(aCard)
>  {
> +  //Application.console.log('loadphoto')

Please get rid of any debugging code.

@@ +926,5 @@
>   */
>  function loadPhoto(aCard)
>  {
> +  //Application.console.log('loadphoto')
> +  var type = aCard.getProperty("PhotoType", "");

We should use let, not var.

@@ +934,5 @@
> +    type = "generic";
> +    gPhotoHandlers[type].onLoad(aCard, document);
> +  }
> +
> +  gPhotoHandlers[type].onShow(aCard, document, "photo");

It's been a while since I messed around with this stuff (and I'm pretty sure I wrote it), but it seems to be we're doing a lot of checking to ensure that gPhotoHandlers[type] exists and that the onX functions return something truthy, or else we fallback to the generic behaviour.

Why is this line not wrapped in similar checks?

@@ +945,3 @@
>   */
> +function onSwitchPhotoType(photoType, event) {
> +  //Application.console.log('onSwitchPhotoType')

Remove

@@ +948,5 @@
>    if (!gEditCard)
>      return;
>  
> +  // Stop event propagation to the radiogroup command event.
> +  if (event) {

Why do we stop the event? Why does the radiogroup even need to call this function?

@@ +994,5 @@
>   * @return true if the OK button was clicked and a photo was chosen
>   */
> +function browsePhoto(event) {
> +  // Stop event propagation to the radiogroup command event.
> +  event.stopPropagation();

Why do we need to do this?

@@ +1014,5 @@
>    return false;
>  }
>  
> +
> +var photoDownloadUI = (function() {

let, not var. Since this is a global var, it should start with g

@@ +1016,5 @@
>  
> +
> +var photoDownloadUI = (function() {
> +  // Used to store the result of setTimeout() to be able to clear the timeout.
> +  var hideTimeout;

Use let, not var for all of these.

@@ +1026,5 @@
> +  var photo;
> +  var progressContainer;
> +
> +  function onStart() {
> +    progressbar = document.getElementById('PhotoDownloadProgress');

It really shouldn't be necessary to re-get these each time onStart is called. This global object should have lazy getters that memoize the nodes after it's gotten them.

@@ +1034,5 @@
> +    progressContainer = document.getElementById('ProgressContainer');
> +
> +    clearTimeout(hideTimeout);
> +
> +    progressContainer.setAttribute("class", "expanded");

progressContainer.collapsed = false (and the XUL node can have collapsed="true" on it by default)

@@ +1035,5 @@
> +
> +    clearTimeout(hideTimeout);
> +
> +    progressContainer.setAttribute("class", "expanded");
> +    progressbar.mode = "determined";

This never changes as far as I can tell - just set it on the node and leave it.

@@ +1042,5 @@
> +  }
> +
> +  function onSuccess() {
> +    progressbar.hidden = true;
> +    progressLabel.value = "Photo successfully updated";

The disappearance of the progress bar is probably enough to signal this. No need to show / hide a message.

@@ +1050,5 @@
> +  }
> +
> +  function onError(state) {
> +    var msg;
> +    // The messages go into the stringbundle once the rest has been approved

Let's do this now.

@@ +1064,5 @@
> +    }
> +    if (msg) {
> +      progressLabel.value = msg;
> +      progressbar.hidden = true;
> +      hideTimeout = setTimeout(function() {

Not too jazzed about hiding an error message after a timeout. We should only hide error messages after we're sure the user has seen them. So perhaps show it until the user performs another action, like tries to download another photo, or changes photo type.

@@ +1073,5 @@
> +
> +  function onProgress(state, percent) {
> +    progressbar.value = percent;
> +    var msg;
> +    // The messages go into the stringbundle once the rest has been approved

Let's do that now.

@@ +1146,5 @@
> +    var directory = GetDirectoryFromURI(gEditCard.abURI);
> +    directory.modifyCard(aCard);
> +
> +    genericPhotoHandler.onShow(aCard, aDocument, "photo");
> +    //Application.console.log('genericPhotoHandler.onSave');

Please remove debugging cruft.

@@ +1168,5 @@
>      return true;
>    },
>  
>    onShow: function(aCard, aDocument, aTargetID) {
> +    //Application.console.log('show '+photoName);

Remove please.

@@ +1178,5 @@
>      return true;
>    },
>  
>    onSave: function(aCard, aDocument) {
> +    //Application.console.log('filePhotoHandler saving URI: '+photoURI);

Remove please.

@@ +1232,5 @@
>      return true;
>    },
>  
>    onSave: function(aCard, aDocument) {
> +    //Application.console.log('webPhotoHandler saving URI: '+photoURI);

Remove please.

@@ +1254,5 @@
> +      var directory = GetDirectoryFromURI(gEditCard.abURI);
> +      directory.modifyCard(aCard);
> +
> +      webPhotoHandler.onShow(aCard, aDocument, "photo");
> +      //Application.console.log('webPhotoHandler.onSave');

Remove please.

::: mail/components/addrbook/content/abCardOverlay.xul
@@ +448,5 @@
>  
>        <!-- ** Photo Tab ** -->
> +      <hbox id="abPhotoTab">
> +        <groupbox>
> +          <caption label="Current photo"/><!-- LOCALIZE -->

Yep, definitely going to need to localize anything you add. I'm not entirely certain that this label is even necessary though - I think it's meaning / purpose is implied, and the label just adds noise.

@@ +457,2 @@
>          <groupbox flex="1">
> +          <caption label="Assign new photo"/><!-- LOCALIZE &PhotoDesc.label; -->

Also not sure this is necessary, as the list of options is (I believe) pretty self-explanatory.

@@ +457,3 @@
>          <groupbox flex="1">
> +          <caption label="Assign new photo"/><!-- LOCALIZE &PhotoDesc.label; -->
> +          <hbox id="ProgressContainer" align="begin" height="20" class="">

As I mentioned before, I think this should go below the radiogroup. Also, height should not be set here - all style stuff should go into CSS. And the empty class attribute should be removed.

::: mail/themes/windows/mail/addrbook/cardDialog.css
@@ +82,5 @@
>    list-style-image: url("chrome://messenger/skin/addressbook/icons/contact-generic-tiny.png");
>  }
> +
> +#ProgressContainer {
> +  max-height: 0;

Neat effect. I'm not sure the border-bottom is necessary if we move the progress display below the radiogroup. With enough margin-top, a line might not be needed at all (lines just add more noise).

I also think the transition is a bit strange because nothing else in the card viewer, let alone the address book, uses such transitions. It's a bit inconsistent, so I think we should take it out for now.

Perhaps in a follow-up bug, we can add more transitions to the address book - but just having one is a bit strange.

So I think you can just use collapsed on the node to hide / show it.
Attachment #775335 - Flags: review?(mconley) → review-
This is still an issue in TB 17.0.8. Is this likely to be completed anytime soon?
Sorry this will never get into the 17.x branch and quite probably not into the soon-to-be-released TB24.

Maybe Samuel could produce a simple alternate version for TB24.0.x that would just catch the exception and make the OK button work (but maybe the image would get lost in that case). But that is a bonus.
What's the deadline for the 24.x branch?
first beta being cut today.  Your making great efforts but I don't see something like this being approved for inclusion in final beta with potentially only a week or two of testing. But I'd think it could make a future point release of 24.
The deadline for features and big fixes (like this one) was 7 weeks ago. TB24 is in beta now. So only if you could produce a 5-10 line patch that just mitigates this problem in an easily understandable way (without the full fix) - that one would have a chance to get into TB24 or its point releases.
The "easily understandable way" would need to be something like: "catch the exception, show it in the error console (with Components.utils.reportError()) and discard the image as the user can reattach it later".
(In reply to :aceman from comment #36)
> The "easily understandable way" would need to be something like: "catch the
> exception, show it in the error console (with
> Components.utils.reportError()) and discard the image as the user can
> reattach it later".

This sounds potentially like creating a new problem in addition to also 'not fixing' another.

If the user is saving a contact entry with an associated image, it follows that the expectation would be to have it preserved without having to check or otherwise think twice about it.

Even if the user is prompted to go back and reattach the image, it could still be a problem if the image is no longer available from elsewhere; especially if the only copy is removed from TB local store/cache.

If the save takes place without a user prompt or warning, then again it follows that the Bugzilla will be filled with yet another bug to follow up (and this one has been duplicated many times already).

Please let's try fix it for good.
(In reply to xanda from comment #37)
> Even if the user is prompted to go back and reattach the image, it could
> still be a problem if the image is no longer available from elsewhere;
> especially if the only copy is removed from TB local store/cache.
I find this case better than blocking the editing of the whole contact (as it is now), forcing the user to delete it and create a new one. But I am not a user of the feature so it is up to you to decide.

> If the save takes place without a user prompt or warning, then again it
> follows that the Bugzilla will be filled with yet another bug to follow up
> (and this one has been duplicated many times already).
> 
> Please let's try fix it for good.
Sure, but then you will wait for the perfect fix until TB31.
(In reply to :aceman from comment #38)
> (In reply to xanda from comment #37)
> > Even if the user is prompted to go back and reattach the image, it could
> > still be a problem if the image is no longer available from elsewhere;
> > especially if the only copy is removed from TB local store/cache.
> I find this case better than blocking the editing of the whole contact (as
> it is now), forcing the user to delete it and create a new one. But I am not
> a user of the feature so it is up to you to decide.

Personally, I would say no it isn't because it's not really changing any of the existing behaviour: users can already remove the association of the image by setting the contact to use the generic option (as opposed to one on disk); after which the whole contact saves upon clicking 'OK'.

When this happens though, the image is deleted from the profile directory (at %userprofile%\AppData\Roaming\Thunderbird\Profiles\xyz123.default\Photos) never to be recovered (?)

Even a tech savvy user, who has the sense to grab a copy of it beforehand, is not going to be spared the tedium of having to find it first. This is made difficult by the name of the image on disk being incorrectly reported in the contacts editor e.g. temp%201.jpg

Regular users are going to be scratching their heads - to say the least.

> > If the save takes place without a user prompt or warning, then again it
> > follows that the Bugzilla will be filled with yet another bug to follow up
> > (and this one has been duplicated many times already).
> > 
> > Please let's try fix it for good.
> Sure, but then you will wait for the perfect fix until TB31.

Not being a developer (any more) I can't really comment on the aforementioned code solutions. That said, it does strike as odd that something so basic has already had x number of iterations (since TB was born) to get right.

Perhaps off-topic now, but talking from more of a business perspective, many of us are coming to rely on TB to provide a full-feature PIM experience. Given Microsoft's insistence on flogging Outlook beyond the budget of most home users' coupled with the fact that many still refuse to yield to cloud based services, this kind of requirement really is a must to get right. Many smartphone manufacturers are now providing full sync suites/integration with FireFox; if issues like this were closed, it seems likely that they would do the same with Thunderbird.

:-)
The problems is the file at 
%userprofile%\AppData\Roaming\Thunderbird\Profiles\xyz123.default\Photos seems to not be used anyway and TB looks for it in the original source folder. If it is not found, the OK does not work.

(In reply to xanda from comment #39)
> Personally, I would say no it isn't because it's not really changing any of
> the existing behaviour: users can already remove the association of the
> image by setting the contact to use the generic option (as opposed to one on
> disk); after which the whole contact saves upon clicking 'OK'.
The point here is the user does not know the image is the problem. So he will not set the "generic" option by himself.
I have a fix ready for the short term: bug 904870
See Also: → 904870
Just wondering if the loaded photos for each contact could be stored inside the address book file, so that if we export it, the photos come with (inside) the file.
Samuel, can you move forward with the patch? Looks like you just need to address comment 30.
Flags: needinfo?(samuel.mueller)
Attached patch Bug-691141.patch (obsolete) — Splinter Review
Finally, I found time to finish this piece of work. 

There have been some UI changes as suggested by mconley. Also, I added the feature to assign a photo by drag and drop.
Attachment #775335 - Attachment is obsolete: true
Flags: needinfo?(samuel.mueller)
Attachment #8542522 - Flags: review?(mconley)
Thanks for the patches, Samuel! Just came off holidays, and I will have a review for you soon.
Mconley, any time to look at this now? :)
Flags: needinfo?(mconley)
Gah, this has really languished, hasn't it?

(In reply to :aceman from comment #46)
> Mconley, any time to look at this now? :)

I don't think so. Do you? (redirects request)
Flags: needinfo?(mconley)
Attachment #8542522 - Flags: review?(mconley) → review?(acelists)
Yes I will look at it.
I am playing with this now and it needs some refreshing.

Due to merge day tomorrow, we need to land at least the strings now and hopefully I can review/finish the code in the next days.
Flags: needinfo?(jorgk)
Do these errors need a full stop at the end?
errorInvalidUri=Error: Invalid source image.
errorNotAvailable=Error: The file not accessible.
errorInvalidImage=Error: Only JPG, PNG and GIF image types are supported.
errorSaveOperation=Error: Could not save the image.
Flags: needinfo?(acelists)
And I can't land the string removal of PhotoDesc.label or something will break.
I think the stops can stay. Or maybe you can convert them to full sentences. E.g. the string "The file not accessible." may be missing an 'is'.
Yes, do not remove the one string, we can do that together with the code part.
Flags: needinfo?(acelists)
Attached patch Bug-691141.patch (strings only) (obsolete) — Splinter Review
Flags: needinfo?(jorgk)
No three dots but a ellipsis.
Attachment #8958041 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c5590822b9cd
Improved AB photo handling (strings only). r=aceman
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Keywords: leave-open
Comment on attachment 8958054 [details] [diff] [review]
Bug-691141.patch -strings only

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

Thanks.
Attachment #8958054 - Flags: review+
Attached patch 691141.patch v2 (obsolete) — Splinter Review
Samuel, thanks for the great code. I'm sorry it took so long to get back to this (unfortunately mconley left TB).
I've made some polishing in the patch. If you are still around you could check my changes (you can compare the patches using bugzilla).

I have played with this and it seems to me this works fine now so I give review+.

This can still be landed together with bug 892889 in TB 60 beta.

Paenglab, can you check if this still looks OK on all platforms? Especially the drag and drop on the marked area.

Patch needs to be applied on top of bug 892889.
Attachment #8542522 - Attachment is obsolete: true
Attachment #8542522 - Flags: review?(acelists)
Attachment #8960322 - Flags: ui-review?(richard.marti)
Attachment #8960322 - Flags: review+
When the contact has a photo and I change it, it still shows the old one until I close the dialog. The new photo should be shown when it's ready.

Aceman, have you tried the download progress? Does it show the text?
(In reply to Richard Marti (:Paenglab) from comment #59)
> When the contact has a photo and I change it, it still shows the old one
> until I close the dialog. The new photo should be shown when it's ready.

I do not see that. If there is a photo and I choose another one via "On this Computer", the new image shows, and even in the contact preview pane in the main AB window, immediatelly.

> Aceman, have you tried the download progress? Does it show the text?
The progress usually just flashes away.
But yes, it does show a text if there is an error. E.g. try choosing and .ico file, or put a link to an image while TB is offline.
On Mac and Windows I get

NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIAbDirectory.modifyCard] abCard.js:1233
	cbSuccess chrome://messenger/content/addressbook/abCard.js:1233:7
	onStateChange chrome://messenger/content/addressbook/abCommon.js:1209:13
	goEditCardDialog chrome://messenger/content/addressbook/abCommon.js:742:3
	AbEditCard chrome://messenger/content/addressbook/abCommon.js:497:5
	AbResultsPaneDoubleClick chrome://messenger/content/addressbook/addressbook.js:525:3
	AbResultsPaneOnClick chrome://messenger/content/addressbook/abResultsPane.js:298:7
	onclick chrome://messenger/content/addressbook/addressbook.xul:1:1

When using the file picker or the drag-n-drop.
Attached patch 691141.patch v2.1 (obsolete) — Splinter Review
Thanks for finding out the error happens when editing a card from the All Addressbooks node.
We already had code to determine the real AB so just use it. All addressbooks didn't exist at the time this patch was written.
Attachment #8960322 - Attachment is obsolete: true
Attachment #8960322 - Flags: ui-review?(richard.marti)
Attachment #8960377 - Flags: ui-review?(richard.marti)
When I add/change a photo and then exit the dialog with "Cancel" the photo is still applied. All other things are reverted. This should also be the case for photos.
Comment on attachment 8960377 [details] [diff] [review]
691141.patch v2.1

Removing the ui-r? until comment 63 is addressed. When the old photo is no more reachable for the user on his computer or somewhere, he can't re-add it again. Also when it is only in the Photo directory in the profile, it's not reachable as the user doesn't know of this directory.
Attachment #8960377 - Flags: ui-review?(richard.marti)
Yes, the current patch saves the changes to the card and the new photo immediately as it is chosen in the dialog.
It takes some more infrastructure to keep the new photos (or more of them) 'at side' and only commit them to the card when the OK button is clicked. If Cancel is clicked all the temporary photo files in Photos/ have to be removed.

I played with it and have the changes almost done.

What do you mean with the reachability of the old photo? Does the current patch still have the problem? If original photo file was removed since and the user opens the card, does not change the photo and saves it, is there a problem (which was the point of this bug)?
I mean, when the user had chosen a photo and then deleted the original or does no more know the link to it.
Yes, and what is the problem with this case? We intend this to work (it didn't in the past therefore we have this bug) also after this rewrite. The photo is "cached" in the Photos directory in the profile so it should be used until user sets a new one from another location. All photos local or remote will get a copy of them caches in Photos directory so they do not need to be accessible after the card is saved.
Yes, when it is changed also when the user cancelled the change, he'd need to search the old photo in the Photos directory. An average user wouldn't know where it is.

But we can stop this discussion as you have the fix almost done.
Attached patch 691141.patch v3 (obsolete) — Splinter Review
Added the cancel possibility. Please hammer on it as much as you can. It should handle various combinations, like having a photo set, then toggling to generic, then to web URL and even then being able to cancel the dialog.

I also tested that when you set a photo and save the card, you can them remove the original image from the filesystem.
When entering the card dialog again, the image should stay. Only if you toggle e.g. to generic and then back to the "on this computer", the image should get reread from the filesystem.
Whenever loading from the filesystem or the web URL fails, the radio should switch back to the generic photo.
Attachment #8960377 - Attachment is obsolete: true
Attachment #8968086 - Flags: ui-review?(richard.marti)
Comment on attachment 8968086 [details] [diff] [review]
691141.patch v3

(In reply to :aceman from comment #69)
> Created attachment 8968086 [details] [diff] [review]
> 691141.patch v3
> 
> Added the cancel possibility. Please hammer on it as much as you can. It
> should handle various combinations, like having a photo set, then toggling
> to generic, then to web URL and even then being able to cancel the dialog.

I tried different things and it worked with using the old image when cancelling.

> I also tested that when you set a photo and save the card, you can them
> remove the original image from the filesystem.
> When entering the card dialog again, the image should stay. Only if you
> toggle e.g. to generic and then back to the "on this computer", the image
> should get reread from the filesystem.

Sounds good.

> Whenever loading from the filesystem or the web URL fails, the radio should
> switch back to the generic photo.

What about returning to the last selected item, instead the generic? Naturally only when it isn't already the one that fails.
Attachment #8968086 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch 691141.patch v3.1 (obsolete) — Splinter Review
I have reviewed Samuel's code, but I have done extensive changes to allow for the cancelling to work (e.g. split the API of the onSave() function into onRead() and onSave()). So for proper procedure, Jorg please glance these changes over after me.

We discussed the patch on IRC and decided to land this as is because of pressure from bug 1453592 which would bitrot this and complicate backporting to TB 60. We can improve the behaviour from comment 70 in a followup after bug 1453592 has landed on top of this.
Attachment #8968086 - Attachment is obsolete: true
Attachment #8969942 - Flags: ui-review+
Attachment #8969942 - Flags: review?(jorgk)
Comment on attachment 8969942 [details] [diff] [review]
691141.patch v3.1

Well, I came a bit late to the party to do a decent review. Since we appear to be under pressure, I tested this a little and it appears to work.

I tried dragging a photo from the web and that doesn't store the URL but rather a funny filename (with .bmp on Windows), which appears to be some temporary file that only exists until the drop.

rs=jorgk.
Attachment #8969942 - Flags: review?(jorgk) → review+
Comment on attachment 8969942 [details] [diff] [review]
691141.patch v3.1

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

OK, you might want to address some nits.

I dragged a flag from www.jorgk.com onto the drop-area and saw a local file with .bmp. That's a little odd, I would have expected the URL of the image.

Maybe you want to look into that now.

::: mail/components/addrbook/content/abCard.js
@@ +1049,5 @@
> +  // child button is pressed. Otherwise, the download is started twice in a row.
> +  if (aEvent)
> +    aEvent.stopPropagation();
> +
> +  const nsIFilePicker = Ci.nsIFilePicker;

Remove this and use Ci.nsIFilePicker instead.

@@ +1094,5 @@
> +    photoType = "file";
> +    document.getElementById("PhotoFile").file = file;
> +  }
> +  // Check if a URL has been dropped.
> +  else {

Unusual indentation, I'd put the comment into the else.

@@ +1101,5 @@
> +      photoType = "web";
> +      document.getElementById("PhotoURI").value = link;
> +    }
> +    // Check if dropped text is a URL.
> +    else {

And here.

@@ +1102,5 @@
> +      document.getElementById("PhotoURI").value = link;
> +    }
> +    // Check if dropped text is a URL.
> +    else {
> +      let link2 = aEvent.dataTransfer.getData("text/plain");

Why a new variable, doesn't 'link' work?
(In reply to Jorg K (GMT+1) from comment #73)
> I dragged a flag from www.jorgk.com onto the drop-area and saw a local file
> with .bmp. That's a little odd, I would have expected the URL of the image.
> Maybe you want to look into that now.
Same happens in a compose window, so the information where the dragged bitmap came from is lost. So just the nits.
Thanks, fixed the nits.
Attachment #8969942 - Attachment is obsolete: true
Attachment #8969955 - Flags: ui-review+
Attachment #8969955 - Flags: review+
Keywords: checkin-needed
Whiteboard: [gs] → [gs][to be applied on top of bug 892889]
Product: MailNews Core → Thunderbird
Comment on attachment 8969955 [details] [diff] [review]
691141.patch v3.2

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

::: mail/components/addrbook/content/abCard.js
@@ +1100,5 @@
> +      document.getElementById("PhotoURI").value = link;
> +    } else {
> +      // Check if dropped text is a URL.
> +      link = aEvent.dataTransfer.getData("text/plain");
> +      if (link.match(/^(ftps?|https?):\/\//)) {

This should be:
if (/^(ftps?|https?):\/\//i.test(link)) {

I'll fix it before landing.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c30d6706a3ba
rework AB contact photo storing UI. ui-r=Paenglab, r=aceman,jorgk
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Comment on attachment 8969955 [details] [diff] [review]
691141.patch v3.2

Note that the landed patch is different.
Attachment #8969955 - Flags: approval-comm-beta+
Depends on: 1458384
Blocks: 1671637
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: