[PATCH V2] bookmarks: prompt to activate new/moved bookmarks on current rev (fixes #2605)

[Angel Ezquerra]

# HG changeset patch
# User Angel Ezquerra <angel.e...@gmail.com>
# Date 1369089117 -7200
# Tue May 21 00:31:57 2013 +0200
# Branch stable
# Node ID ea55ab0edbcbbab1ee9243524087f059812c8b6e
# Parent 7b0dd1c0875c08fcfb5b93ada58957cabf961ed7
bookmarks: prompt to activate new/moved bookmarks on current rev (fixes #2605)

diff --git a/tortoisehg/hgqt/bookmark.py b/tortoisehg/hgqt/bookmark.py
--- a/tortoisehg/hgqt/bookmark.py
+++ b/tortoisehg/hgqt/bookmark.py
@@ -162,7 +162,10 @@
return

def finished():
- self.set_status(_("Bookmark '%s' has been added") % bookmark, True)
+ self._bookmark_action_finished(
+ _("Bookmark '%s' has been added") % bookmark,
+ bookmark,
+ activate_prompt=True)

cmdline = ['bookmarks', '--repository', self.repo.root,
'--rev', str(self.rev), bookmarklocal]
@@ -178,7 +181,10 @@
return

def finished():
- self.set_status(_("Bookmark '%s' has been moved") % bookmark, True)
+ self._bookmark_action_finished(
+ _("Bookmark '%s' has been moved") % bookmark,
+ bookmark,
+ activate_prompt=True)

cmdline = ['bookmarks', '--repository', self.repo.root,
'--rev', str(self.rev), '--force', bookmarklocal]
@@ -224,6 +230,21 @@
self.cmd.run(cmdline)
self.finishfunc = finished

+ def _bookmark_activated(self, bookmark):
+ self.set_status(_('bookmark "%s" activated') % bookmark, True)
+
+ def _bookmark_action_finished(self, msg, ubookmark, activate_prompt=False):
+ self.set_status(msg, True)
+ if activate_prompt and self.rev == self.repo['.'].rev():
+ if qtlib.QuestionMsgBox(_('Activate bookmark?'),
+ _('Do you want to activate the "%s" bookmark?') \
+ % ubookmark, parent=self):
+ bookmark = hglib.fromunicode(ubookmark)
+ cmdline = ['update', '--repository', self.repo.root,
+ '--rev', bookmark]
+ self.cmd.run(cmdline)
+ self.finishfunc = lambda: self._bookmark_activated(bookmark)
+
def reject(self):
# prevent signals from reaching deleted objects
self.repo.repositoryChanged.disconnect(self.refresh)

[Angel Ezquerra]

[Angel Ezquerra]

Sorry for the double post. I thought that the first time I had sent
the patch something had gone wrong. Both V2 patches are identical.

Compared to V1 I have made it so that the "bookmark activated" message
is the same regardless of whether the user added or moved a bookmark,
which makes the message translatable.

BTW, this is meant for stable.

Cheers,

Angel

[Yuya Nishihara]

On Wed, 22 May 2013 18:37:14 +0200, Angel Ezquerra wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.e...@gmail.com>
> # Date 1369089117 -7200
> # Tue May 21 00:31:57 2013 +0200
> # Branch stable
> # Node ID ea55ab0edbcbbab1ee9243524087f059812c8b6e
> # Parent 7b0dd1c0875c08fcfb5b93ada58957cabf961ed7
> bookmarks: prompt to activate new/moved bookmarks on current rev (fixes #2605)

Do we really need a prompt?

When BookmarkDialog was added (around hg 1.8), "hg bookmark -r curr_rev" did
activate new bookmark, so it simply appends -r option even if curr_rev.

But now "hg bookmark -r curr_rev name" is not the same as "hg bookmark name".

> diff --git a/tortoisehg/hgqt/bookmark.py b/tortoisehg/hgqt/bookmark.py
> --- a/tortoisehg/hgqt/bookmark.py
> +++ b/tortoisehg/hgqt/bookmark.py
> @@ -162,7 +162,10 @@
> return
>
> def finished():
> - self.set_status(_("Bookmark '%s' has been added") % bookmark, True)
> + self._bookmark_action_finished(
> + _("Bookmark '%s' has been added") % bookmark,
> + bookmark,
> + activate_prompt=True)

I don't see any reason to move set_status().

[...]

> + def _bookmark_activated(self, bookmark):
> + self.set_status(_('bookmark "%s" activated') % bookmark, True)

"bookmark" isn't a unicode.

> + def _bookmark_action_finished(self, msg, ubookmark, activate_prompt=False):
> + self.set_status(msg, True)
> + if activate_prompt and self.rev == self.repo['.'].rev():
> + if qtlib.QuestionMsgBox(_('Activate bookmark?'),
> + _('Do you want to activate the "%s" bookmark?') \
> + % ubookmark, parent=self):
> + bookmark = hglib.fromunicode(ubookmark)
> + cmdline = ['update', '--repository', self.repo.root,
> + '--rev', bookmark]
> + self.cmd.run(cmdline)
> + self.finishfunc = lambda: self._bookmark_activated(bookmark)

"activate_prompt" is always True.

Regards,

[Angel Ezquerra]

On Thu, May 23, 2013 at 5:00 PM, Yuya Nishihara <yu...@tcha.org> wrote:
> On Wed, 22 May 2013 18:37:14 +0200, Angel Ezquerra wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.e...@gmail.com>
>> # Date 1369089117 -7200
>> # Tue May 21 00:31:57 2013 +0200
>> # Branch stable
>> # Node ID ea55ab0edbcbbab1ee9243524087f059812c8b6e
>> # Parent 7b0dd1c0875c08fcfb5b93ada58957cabf961ed7
>> bookmarks: prompt to activate new/moved bookmarks on current rev (fixes #2605)
>
> Do we really need a prompt?
>
> When BookmarkDialog was added (around hg 1.8), "hg bookmark -r curr_rev" did
> activate new bookmark, so it simply appends -r option even if curr_rev.
>
> But now "hg bookmark -r curr_rev name" is not the same as "hg bookmark name".

Yuya,

Are you suggesting that if the user bookmarks (or moves a bookmark to)
the current revision we should always activate it (i.e. "hg bookmark
FOO" rather than "hg bookmark -r . FOO")?

Angel

[Yuya Nishihara]

Yes, because Mercurial activates it by default (unless he specifies -r
explicitly.)

It'll be possible to provide an option not to activate, but it should be
off by default. (and I'm not sure how useful it is.)

Regards,

[Marc Schlaich]

Here is my version (sorry for double post, posted the wrong patch in previous message). It adds an "activate" checkbox to the bookmark dialog if current rev == selected rev:

# HG changeset patch
# User schlamar <marc.s...@googlemail.com>
# Date 1369319978 -7200
#      Thu May 23 16:39:38 2013 +0200
# Node ID e40ec5f61f1edb9ab37bd8d5b5bb14187dddcc23
# Parent  96275cf8d36474942e299868eefa16deb3bda243
bookmark: option to make bookmarks on current node active (fixes #2605)

diff -r 96275cf8d364 -r e40ec5f61f1e tortoisehg/hgqt/bookmark.py
--- a/tortoisehg/hgqt/bookmark.py         Thu May 23 00:54:27 2013 +0900
+++ b/tortoisehg/hgqt/bookmark.py	Thu May 23 16:39:38 2013 +0200
@@ -62,6 +62,15 @@
         self.newNameEdit.textEdited.connect(self.bookmarkTextChanged)
         form.addRow(_('New Name:'), self.newNameEdit)
 
+        ### Activate checkbox
+        self.activateCheckBox = QCheckBox()
+        if self.node == self.repo['.'].node():
+            self.activateCheckBox.setChecked(True)
+        else:
+            self.activateCheckBox.setChecked(False)
+            self.activateCheckBox.setEnabled(False)
+        form.addRow(_('Activate:'), self.activateCheckBox)
+
         ## bottom buttons
         BB = QDialogButtonBox
         bbox = QDialogButtonBox()
@@ -164,8 +173,10 @@
         def finished():
             self.set_status(_("Bookmark '%s' has been added") % bookmark, True)
 
-        cmdline = ['bookmarks', '--repository', self.repo.root,
-                   '--rev', str(self.rev), bookmarklocal]
+        cmdline = ['bookmarks', '--repository', self.repo.root]
+        if not self.activateCheckBox.isChecked():
+            cmdline.extend(['--rev', str(self.rev)])
+        cmdline.append(bookmarklocal)
         self.cmd.run(cmdline)
         self.finishfunc = finished
 

[Marc Schlaich]

My patch is missing the move action, but that would be trivial to add.

[Angel Ezquerra]

Yuya,

I agree.

Since Marc has shown interest on this, maybe he would rewrite the
patch? It should be much simpler than what I wrote...

Angel

[Angel Ezquerra]

Marc,

as I just said to Yuya, I'm willing to drop my current patch and let
you handle this if you want to.

Your patch (if you do it) should cover all use cases (both move and
add). Personally I think we do not even need an "Activate bookmark"
checkbox. Just activate it when the bookmark is added or moved to the
current revision.

Cheers,

Angel

[Marc Schlaich]

Here is my patch v2 including the move action. I would suggest to keep the activate checkbox, because there could be cases where you actually need it. It is possible that you have already the right bookmark active but want to add another one on the current changeset. If the new one is picked automatically and you are not in a clean working state going back to the right one would be really annoying (you have to merge changes, not added files could be accidentally lost, etc.).

# HG changeset patch
# User schlamar <marc.s...@googlemail.com>
# Date 1369378721 -7200
#      Fri May 24 08:58:41 2013 +0200
# Node ID bfcb425e1915bbe5c0f2c9f8000945c748d9b9b4
# Parent  96275cf8d36474942e299868eefa16deb3bda243
bookmark: option to activate if adding or moving to current node (fixes #2605)

diff -r 96275cf8d364 -r bfcb425e1915 tortoisehg/hgqt/bookmark.py
--- a/tortoisehg/hgqt/bookmark.py         Thu May 23 00:54:27 2013 +0900
+++ b/tortoisehg/hgqt/bookmark.py	Fri May 24 08:58:41 2013 +0200
@@ -62,6 +62,15 @@
         self.newNameEdit.textEdited.connect(self.bookmarkTextChanged)
         form.addRow(_('New Name:'), self.newNameEdit)
 
+        ### Activate checkbox
+        self.activateCheckBox = QCheckBox()
+        if self.node == self.repo['.'].node():
+            self.activateCheckBox.setChecked(True)
+        else:
+            self.activateCheckBox.setChecked(False)
+            self.activateCheckBox.setEnabled(False)
+        form.addRow(_('Activate:'), self.activateCheckBox)
+
         ## bottom buttons
         BB = QDialogButtonBox
         bbox = QDialogButtonBox()
@@ -164,8 +173,10 @@
         def finished():
             self.set_status(_("Bookmark '%s' has been added") % bookmark, True)
 
-        cmdline = ['bookmarks', '--repository', self.repo.root,
-                   '--rev', str(self.rev), bookmarklocal]
+        cmdline = ['bookmarks', '--repository', self.repo.root]
+        if not self.activateCheckBox.isChecked():
+            cmdline.extend(['--rev', str(self.rev)])
+        cmdline.append(bookmarklocal)
         self.cmd.run(cmdline)
         self.finishfunc = finished
 
@@ -180,8 +191,10 @@
         def finished():
             self.set_status(_("Bookmark '%s' has been moved") % bookmark, True)
 
-        cmdline = ['bookmarks', '--repository', self.repo.root,
-                   '--rev', str(self.rev), '--force', bookmarklocal]
+        cmdline = ['bookmarks', '--repository', self.repo.root]
+        if not self.activateCheckBox.isChecked():
+            cmdline.extend(['--rev', str(self.rev)])
+        cmdline.extend(['--force', bookmarklocal])
         self.cmd.run(cmdline)
         self.finishfunc = finished
 

[Yuya Nishihara]

On Fri, 24 May 2013 07:54:51 -0700 (PDT), Marc Schlaich wrote:
> Here is my patch v2 including the move action. I would suggest to keep the
> activate checkbox, because there could be cases where you actually need it.
> It is possible that you have already the right bookmark active but want to
> add another one on the current changeset. If the new one is picked
> automatically and you are not in a clean working state going back to the
> right one would be really annoying (you have to merge changes, not added
> files could be accidentally lost, etc.).

Ok.

> # HG changeset patch
> # User schlamar <marc.s...@googlemail.com>
> # Date 1369378721 -7200
> # Fri May 24 08:58:41 2013 +0200
> # Node ID bfcb425e1915bbe5c0f2c9f8000945c748d9b9b4
> # Parent 96275cf8d36474942e299868eefa16deb3bda243
> bookmark: option to activate if adding or moving to current node (fixes #2605)

Queued for stable, thanks.

> diff -r 96275cf8d364 -r bfcb425e1915 tortoisehg/hgqt/bookmark.py
> --- a/tortoisehg/hgqt/bookmark.py Thu May 23 00:54:27 2013 +0900
> +++ b/tortoisehg/hgqt/bookmark.py Fri May 24 08:58:41 2013 +0200
> @@ -62,6 +62,15 @@
> self.newNameEdit.textEdited.connect(self.bookmarkTextChanged)
> form.addRow(_('New Name:'), self.newNameEdit)
>
> + ### Activate checkbox
> + self.activateCheckBox = QCheckBox()
> + if self.node == self.repo['.'].node():
> + self.activateCheckBox.setChecked(True)
> + else:
> + self.activateCheckBox.setChecked(False)
> + self.activateCheckBox.setEnabled(False)
> + form.addRow(_('Activate:'), self.activateCheckBox)

Personally I like the following UI

[v] Activate
~~~~~~~~
empty space

than

Activate: [v]
~~~
small clickable area

Regards,