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

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Rick Alther]

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.

Comment 1

[Wayne Mery (:wsmwk)]

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

Comment 2

[Rick Alther]

Attachment: listing of ldap_2.servers preferences

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.

Comment 3

[Ludovic Hirlimann [:Usul]]

hum xref 609074.

Comment 4

[Rick Alther]

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.

Comment 5

[Samuel Müller]

Rick, have you assigned a photo from your computer to that contact? If yes, did you delete or rename the photo?

Comment 6

[Rick Alther]

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!

Comment 7

[Wayne Mery (:wsmwk)]

great detective work!
I can also reproduce. Nothing in error console

Comment 8

[Samuel Müller]

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?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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

Comment 10

[Samuel Müller]

Attachment: 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

Comment 11

[Mike Conley (: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!

Comment 12

[Samuel Müller]

Attachment: mockup of proposed ui changes

while a download is going on, the photo is hidden to make space for the new elements.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 568060 [details]
mockup of proposed ui changes

Tossing over to bwinton for ui-review / feedback.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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

Comment 15

[Samuel Müller]

Attachment: mockup of proposed ui changes

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.

Comment 16

[Samuel Müller]

(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.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

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

Comment 18

[Samuel Müller]

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?

Comment 21

[Raül Trias]

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!

Comment 22

[Samuel Müller]

Attachment: 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!

Comment 23

[:aceman]

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.

Comment 24

[Samuel Müller]

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.

Comment 25

[:aceman]

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.

Comment 26

[Samuel Müller]

Attachment: async download of contact photos. requires applied patch from bug bug 892889.

calls to application.console.log will be removed in the final version.

Comment 27

[:aceman]

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?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

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 30

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 31

[xanda]

This is still an issue in TB 17.0.8. Is this likely to be completed anytime soon?

Comment 32

[:aceman]

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.

Comment 33

[Samuel Müller]

What's the deadline for the 24.x branch?

Comment 34

[Wayne Mery (:wsmwk)]

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.

Comment 35

[:aceman]

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.

Comment 36

[:aceman]

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".

Comment 37

[xanda]

(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.

Comment 38

[:aceman]

(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.

Comment 39

[xanda]

(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.

:-)

Comment 40

[:aceman]

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.

Comment 41

[Samuel Müller]

I have a fix ready for the short term: bug 904870

Comment 42

[Marco A.G.Pinto [:marcoagpinto]]

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.

Comment 43

[:aceman]

Samuel, can you move forward with the patch? Looks like you just need to address comment 30.

Comment 44

[Samuel Müller]

Attachment: Bug-691141.patch

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.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Thanks for the patches, Samuel! Just came off holidays, and I will have a review for you soon.

Comment 46

[:aceman]

Mconley, any time to look at this now? :)

Comment 47

[Mike Conley (: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)

Comment 48

[:aceman]

Yes I will look at it.

Comment 49

[:aceman]

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.

Comment 50

[Jorg K (CEST = GMT+2)]

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.

Comment 51

[Jorg K (CEST = GMT+2)]

And I can't land the string removal of PhotoDesc.label or something will break.

Comment 52

[:aceman]

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.

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: Bug-691141.patch (strings only)

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: Bug-691141.patch (strings only, take two, no added newline)

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: Bug-691141.patch -strings only

No three dots but a ellipsis.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5590822b9cd
Improved AB photo handling (strings only). r=aceman

Comment 57

[:aceman]

Comment on attachment 8958054 [details] [diff] [review]
Bug-691141.patch -strings only

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

Thanks.

Comment 58

[:aceman]

Attachment: 691141.patch v2

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.

Comment 59

[Richard Marti (:Paenglab)]

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?

Comment 60

[:aceman]

(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.

Comment 61

[Richard Marti (:Paenglab)]

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.

Comment 62

[:aceman]

Attachment: 691141.patch v2.1

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.

Comment 63

[Richard Marti (:Paenglab)]

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 64

[Richard Marti (:Paenglab)]

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.

Comment 65

[:aceman]

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)?

Comment 66

[Richard Marti (:Paenglab)]

I mean, when the user had chosen a photo and then deleted the original or does no more know the link to it.

Comment 67

[:aceman]

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.

Comment 68

[Richard Marti (:Paenglab)]

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.

Comment 69

[:aceman]

Attachment: 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 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.

Comment 70

[Richard Marti (:Paenglab)]

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.

Comment 71

[:aceman]

Attachment: 691141.patch v3.1

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.

Comment 72

[Jorg K (CEST = GMT+2)]

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.

Comment 73

[Jorg K (CEST = GMT+2)]

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?

Comment 74

[Jorg K (CEST = GMT+2)]

(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.

Comment 75

[:aceman]

Attachment: 691141.patch v3.2

Thanks, fixed the nits.

Comment 76

[Jorg K (CEST = GMT+2)]

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.

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c30d6706a3ba
rework AB contact photo storing UI. ui-r=Paenglab, r=aceman,jorgk

Comment 78

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969955 [details] [diff] [review]
691141.patch v3.2

Note that the landed patch is different.

Comment 79

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 5):
https://hg.mozilla.org/releases/comm-beta/rev/5f19482796a3b598893add08b18ae7cb42661af8

Comment 80

[Óvári]

Does this fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=548266#c5
Thank you