[Vimperator] [Patch] Issue 320: Bookmark Folders/Subfolders

[teramako]

Hi

I'm now creating a patch add "-folder" option to :bmark command.

http://code.google.com/p/vimperator-labs/issues/detail?id=320#c7

Could you review my patch ?


Thanks.

[Jostein Berntsen]

Post by teramako
Hi
I'm now creating a patch add "-folder" option to :bmark command.
http://code.google.com/p/vimperator-labs/issues/detail?id=320#c7
Could you review my patch ?

This looks like a great new option! I tried to apply the patch, but received
"Hunk 1-5 failed". Could you indicate how this patch should be applied?

Jostein

[Jostein Berntsen]

Post by Jostein Berntsen

Post by teramako
Hi
I'm now creating a patch add "-folder" option to :bmark command.
http://code.google.com/p/vimperator-labs/issues/detail?id=320#c7
Could you review my patch ?

This looks like a great new option! I tried to apply the patch, but received
"Hunk 1-5 failed". Could you indicate how this patch should be applied?

Sorry, seems like I patched this to the wrong version. Found the right one
and it patched as expected. The folder option works perfectly for me after
some quick tests. Thanks!

Jostein

[Martin Stubenschrott]

Post by teramako
I'm now creating a patch add "-folder" option to :bmark command.
http://code.google.com/p/vimperator-labs/issues/detail?id=320#c7
Could you review my patch ?

Hi,

thanks for the work. From a static POV it looks good, with some comments:

1.) there are still some liberator.log() left in the code which should be
removed
2.) The short option should be -f and not -F
3.) The folder separator with / is fine to me, no need to hack around
people using that as a folder name.
4.) A NEWS entry and updated help document is missing
5.) At the end of the patch, in completion.bookmarkFolder, there are many
var folder = ...; Isn't "let" the new "var" or is it needed for some
special thing?
6.) If the bookmark folder cannot be found, the bookmark shouldn't be added
to the top folder, but rather an error thrown to the user.

Anyway, if those are fixed, you can commit the patch, and thanks a lot!

--
Martin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mozdev.org/pipermail/vimperator/attachments/20120516/8b09d92b/attachment.html>

[teramako]

Post by Martin Stubenschrott

Post by teramako
I'm now creating a patch add "-folder" option to :bmark command.
http://code.google.com/p/vimperator-labs/issues/detail?id=320#c7
Could you review my patch ?

Hi,

Thank you for feedback.

I committed
http://code.google.com/p/vimperator-labs/source/detail?r=c410c1d10413

Post by Martin Stubenschrott
1.) there are still some liberator.log() left in the code which should be
removed

fixed

Post by Martin Stubenschrott
2.) The short option should be -f and not -F

fixed

Post by Martin Stubenschrott
3.) The folder separator with / is fine to me, no need to hack around
people using that as a folder name.
4.) A NEWS entry and updated help document is missing

updated NEW and help (marks.xml)

Post by Martin Stubenschrott
5.) At the end of the patch, in completion.bookmarkFolder, there are many
var folder = ...; Isn't "let" the new "var" or is it needed for some
special thing?

It seems that top-level LetDeclarations are converted to
VariableDeclarations on Gecko
:echo (function(){ let a = 0; }).toString()
=> function(){ var a = 0; }

but anyway, fixed

Post by Martin Stubenschrott
6.) If the bookmark folder cannot be found, the bookmark shouldn't be added
to the top folder, but rather an error thrown to the user.

fixed

Best regards.