242545 - Bookmark import should give user feedback

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

Right now, importing bookmarks gives no status response of any kind. A user
might easily think the import failed, especially since the imported folder is
only visible in the bookmark manager (and not the menu).

We should give the user feedback like "Bookmarks successfully imported into
<bookmark folder>" or "Error during bookmark import. Some bookmarks may not have
been imported".

Comment 1

[Stuart Morgan]

Nominating for 1.0 polish

Comment 2

[Stuart Morgan]

Bumping up to 0.8. A number of people trying 0.8b thought (not surprisingly)
that bookmark import failed. Since the ability to have all the bookmarks they
are used to is vital for many people, this is a big barrier to people trying or
switching to Camino.

Comment 3

[Stuart Morgan]

Attachment: gives import feedback

Changes:
-Import is always a panel, rather than a sheet (since it's unrelated to the
browser window)
-A feedback sheet is displayed after import. On success, it explains where the
user can find their newly imported bookmarks (as it's not immediately obvious)

Comment 4

[Stuart Morgan]

Attachment: updated localizable strings

Adds localizable strings for import feedback. Removes some unused strings that
look like they were intended for giving user feedback (but apparently never
were).

Comment 5

[Stuart Morgan]

Attachment: slight nib modifications

Better positioning, adjusted slightly to be better suited to non-sheet display.

Comment 6

[Josh Aas]

Comments:

- please center the bm import window when it comes up (using NSWindow's "center"
method). it should not just be at a somewhat random location in the top left.
- "Select a File" should not have the word "File" capitalized - general convention
- perhaps the open panel for "Select a File..." should appear as a sheet on the
bm import window not another window.
- change the "Open" default button in the open panel (sheet?) to "Import" since
clicking "Open" actually does import, not open the file
- what about instead of showing the user some text in a sheet telling them where
their bookmarks went, instead open a new window, open the bm manager in it, and
hilight the imported bookmarks collection? show don't tell (quote from brion)

Nice work so far. With these changes it'll be perfect I think.

Comment 7

[Stuart Morgan]

> - please center the bm import window when it comes up (using NSWindow's
> "center" method). it should not just be at a somewhat random location
> in the top left.
> - "Select a File" should not have the word "File" capitalized - general
> convention
> - perhaps the open panel for "Select a File..." should appear as a sheet
> on the bm import window not another window.

Sure

> - change the "Open" default button in the open panel (sheet?) to "Import" 
> since clicking "Open" actually does import, not open the file

Is that possible? I'm not seeing any way to modify an NSOpenPanel's buttons.

> - what about instead of showing the user some text in a sheet telling
> them where their bookmarks went, instead open a new window, open the
> bm manager in it, and hilight the imported bookmarks collection? show
> don't tell (quote from brion)

Sounds good to me.

Updated patch soon. Thanks for the good comments.

Comment 8

[Mike Pinkerton (not reading bugmail)]

use |-setPrompt:| (on NSSavePanel, which a NSOpenPanel derives from) to set the
title of the "OK" button. Had to dig for this myself :)

Comment 9

[Stuart Morgan]

Attachment: updated patch

Addresses all of Josh's comments except making the open panel a sheet. For some
reason, that behaved extremely badly (disconnecting subsequent sheets from the
import panel, leaving a dead input panel sometimes, etc.) and I figured the
difference is minor enough not to be worth them time right now.

Comment 10

[Stuart Morgan]

Attachment: new strings

Removed the confirmation strings, since they are no longer needed.

Comment 11

[Stuart Morgan]

Attachment: newer nib

Fixes capitalization issues, uses an NSWindow instead of NSPanel so that it
shows up in the window menu and thus can't be lost so easily.

Comment 12

[Josh Aas]

Patch should be in "diff -u" format, and it doesn't apply as cleanly as it might:

patching file src/bookmarks/BookmarkViewController.mm
Hunk #1 succeeded at 791 (offset 18 lines).

Haven't compiled it yet.

Comment 13

[Jasper]

I really think a progress bar of some kind will be needed when we are loading
large (bug 236373) bookmarks files. We do not want our users to have a
non-fuctional app for more the 4 seconds unless we display some kind of
indicator of how long they have to wait. I know it's not easy, but it's best I
think.

Comment 14

[Ludovic Hirlimann [:Usul]]

(In reply to comment #13)
> I really think a progress bar of some kind will be needed when we are loading
> large (bug 236373) bookmarks files. We do not want our users to have a
> non-fuctional app for more the 4 seconds unless we display some kind of
> indicator of how long they have to wait. I know it's not easy, but it's best I
> think.

I agree but lets implemet this as a RFE for 1.0.

Comment 15

[Stuart Morgan]

> Patch should be in "diff -u" format, and it doesn't apply as cleanly
> as it might:

Sorry, I keep forgetting (this is -c). The unclean-ness is probably because
there was a tiny bit of file overlap with my bookmark loading patch. I can post
a new patch if need be.

Jasper:
We can easily add a spinner progress indicator once 10.1 support is gone, so
after this lands I can make a new trunk-only additional patch to add a spinner.
I'd just rather this be self-contained and not get into trunk/branch differences.

Comment 16

[Ludovic Hirlimann [:Usul]]

Comment on attachment 149534 [details] [diff] [review]
updated patch

This is good enough for 0.8. We'll need something better from the UI point of
view for 1.0.

Comment 17

[Jasper]

I was actually thinking of a real progress bar / barber pole ;)
10.1 also has a spinner, it just looked different, 2 arrows chasing each other :)

It's up to you.

Comment 18

[Josh Aas]

Please repost the patch in "diff -u" format. I don't know how to interpret some
of the markings in whatever patch style you used.

Comment 19

[Stuart Morgan]

Attachment: diff -u version

Comment 20

[Stuart Morgan]

Comment on attachment 149657 [details] [diff] [review]
diff -u version

diff -u version, as requested.

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 149657 [details] [diff] [review]
diff -u version

sr=pink

landed on trunk and branch

Comment 22

[Mike Pinkerton (not reading bugmail)]

fixed, take other improvement ideas to a new bug