450318 - No error on duplicate calendar address, stalls

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Tyson Vanover]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Thunderbird 2.0.0.16 (20080708), Lightning 0.8 (2008033120), Dgata 0.4

When adding a new network calendar, if you add the same address twice there is no error message.  Once you reach the screen where you enter the calendar name the next button activates but pushing it accomplishes nothing and there is no error warning you of what has gone wrong.

Reproducible: Always

Steps to Reproduce:
1.create new calendar
2.choose network calender
3.paste address of .xml calender and choose calender source (google in this case)
4.enter name of calender and assign color
5.repeat 1-4 again with the same calender address and source but name and color different
Actual Results:  
applications seems to become unresponsive

Expected Results:  
A popup warning that you already have added this calender and preferably what the local calendar is named.  Then redirection back to the page where the user provides the link.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This is not a gdata-only bug, moving accordingly. I stumbled across this too, but since we are already in string freeze, we can't take this for the release. See related bug 314594, I thought there was also a general bug on error notifications in the new calendar wizard, but I couldn't find it.

Comment 2

[Stefan Sitter [:ssitter]]

Did Bug 448206 improved situation in recent 0.9pre nightly builds?

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This adds some simple error handling to the dialog. We might want to go for a more complete solution in a different bug, but this at least tells the user why the "Next" button is disabled.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Screenshot for ui-review

I'm happy to change colors if needed, but these use the system style for the <notification> element.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 432575 [details]
Screenshot for ui-review

I haven't tried this out yet, but does the "Please enter a valid location" appear while you are typing in a valid location?  I wouldn't think that's exactly ideal but it's simple and works.  Otherwise things look good.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #5)
> (From update of attachment 432575 [details])
> I haven't tried this out yet, but does the "Please enter a valid location"
> appear while you are typing in a valid location?  I wouldn't think that's
> exactly ideal but it's simple and works.  Otherwise things look good.

Unfortunately, yes. I could postpone the checking a bit (i.e if no key was pressed for 500ms, then check, or on blur), that should work out.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

This patch uses a search textbox to delay the checking for 500ms and also contains the file I forgot to hg add.

Comment 8

[Markus Adrario [:Taraman]]

Comment on attachment 433788 [details] [diff] [review]
Fix - v2

Patch looks good and works

One nit:
>+        let notificationbox = document.getElementById("location-notifications");
>         if (canAdvance && document.getElementById("calendar-uri").value &&
>             curPage.pageid == "locationPage") {
>-            canAdvance = checkURL();
>+            let [reason,] = parseUri(document.getElementById("calendar-uri").value);
>+            canAdvance = (reason == errorConstants.SUCCESS);
>+            setNotification(reason);
>+        } else {
Although indentation is correct here, it somewhat conceals the end of the if statement. Maybe you can indent the second line of it a little more.

Due to Bug 533220 it may be a good time to move these files to calendar/base/content/dialogs, since the code is cleaned up now.
If you prefer, I can prepare a seperate patch in a first step to fix Bug 533220.
This would include renaming to calendar-creation-wizard.xul and .js

With the current patch
r=markus

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #8)
> Although indentation is correct here, it somewhat conceals the end of the if
> statement. Maybe you can indent the second line of it a little more.
Done

> Due to Bug 533220 it may be a good time to move these files to
> calendar/base/content/dialogs, since the code is cleaned up now.
> If you prefer, I can prepare a seperate patch in a first step to fix Bug
> 533220.
Yes, I'd prefer fixing this in the other bug. I wouldn't consider the code cleaned up there, but its good enough for a start :-)

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/cb2333d71134>
-> FIXED