Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gitk: add right-click context menu for tags #866

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adlternative
Copy link

@adlternative adlternative commented Feb 4, 2021

This patch want to fix:
#855

We can provide for right-clicking the tag icon in gitk
Rename this tag", "Remove this tag", "Copy tag name" function.

For convenience, only the tags on the branch with the number of
tags <=3 are processed temporarily.

Thanks!

cc: Paul Mackerras [email protected]
cc: Anders Kaseorg [email protected]
cc: Junio C Hamano [email protected]

@adlternative
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 4, 2021

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-866/adlternative/gitk_tag_new_opt-v1

To fetch this version to local tag pr-866/adlternative/gitk_tag_new_opt-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-866/adlternative/gitk_tag_new_opt-v1

Copy link

@kxrob kxrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing. I suggest these changes below to get a correct view refresh. Works for me so far:

--- gitk
+++ gitk
@@ -1894,14 +1894,6 @@
     unset headids($name)
 }
 
-proc movetag {id name} {
-    global tagids idtags
-
-    removetag $tagids($name) $name
-    set tagids($name) $id
-    lappend idtags($id) $name
-}
-
 proc removetag {id name} {
     global tagids idtags
 
@@ -9601,7 +9593,7 @@
 
     set val(name) $tagmenutag
     set val(id) $tagmenuid
-    set val(command) [list mvtago $top $tagmenutag]
+    set val(command) [list mvtaggo $top $tagmenutag]
 
     set ui(title) [mc "Rename tag %s" $tagmenutag]
     set ui(accept) [mc "Rename"]
@@ -9695,8 +9687,8 @@
     focus $top.name
 }
 
-proc mvtago {top prevname} {
-    global tagids idheads mainhead mainheadid
+proc mvtaggo {top prevname} {
+    global tagids idtags idheads mainhead mainheadid
 
     set name [$top.name get]
     set id [$top.sha1 get]
@@ -9712,6 +9704,8 @@
     nowbusy renametag
     update
     if {[catch {
+        # NOTE: for an annotated tag, the new tag points to the old tag object
+        # where the old primary tag name is still recorded inside. Acceptable.
         eval exec "git tag $name $prevname"
         eval exec "git tag -d $prevname"
     } err]} {
@@ -9722,6 +9716,9 @@
         removetag $id $prevname
         set tagids($name) $id
         lappend idtags($id) $name
+        redrawtags $id
+        addedtag $id
+        dispneartags 0
         run refill_reflist
     }
 
@@ -10190,6 +10187,7 @@
         return
     }
     removetag $id $tag
+    redrawtags $id
     notbusy rmtag
     run refill_reflist
 }

@adlternative
Copy link
Author

Thanks for implementing. I suggest these changes below to get a correct view refresh. Works for me so far:

--- gitk
+++ gitk
@@ -1894,14 +1894,6 @@
     unset headids($name)
 }
 
-proc movetag {id name} {
-    global tagids idtags
-
-    removetag $tagids($name) $name
-    set tagids($name) $id
-    lappend idtags($id) $name
-}
-
 proc removetag {id name} {
     global tagids idtags
 
@@ -9601,7 +9593,7 @@
 
     set val(name) $tagmenutag
     set val(id) $tagmenuid
-    set val(command) [list mvtago $top $tagmenutag]
+    set val(command) [list mvtaggo $top $tagmenutag]
 
     set ui(title) [mc "Rename tag %s" $tagmenutag]
     set ui(accept) [mc "Rename"]
@@ -9695,8 +9687,8 @@
     focus $top.name
 }
 
-proc mvtago {top prevname} {
-    global tagids idheads mainhead mainheadid
+proc mvtaggo {top prevname} {
+    global tagids idtags idheads mainhead mainheadid
 
     set name [$top.name get]
     set id [$top.sha1 get]
@@ -9712,6 +9704,8 @@
     nowbusy renametag
     update
     if {[catch {
+        # NOTE: for an annotated tag, the new tag points to the old tag object
+        # where the old primary tag name is still recorded inside. Acceptable.
         eval exec "git tag $name $prevname"
         eval exec "git tag -d $prevname"
     } err]} {
@@ -9722,6 +9716,9 @@
         removetag $id $prevname
         set tagids($name) $id
         lappend idtags($id) $name
+        redrawtags $id
+        addedtag $id
+        dispneartags 0
         run refill_reflist
     }
 
@@ -10190,6 +10187,7 @@
         return
     }
     removetag $id $tag
+    redrawtags $id
     notbusy rmtag
     run refill_reflist
 }

Thanks for the patch!
This made me understand "redrawtags" and "dispneartags"
should be functions used to refresh tags.

@adlternative adlternative changed the title [RFC] gitk: tag add right click options gitk: tag add right click options Feb 16, 2021
@adlternative
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 16, 2021

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-866/adlternative/gitk_tag_new_opt-v2

To fetch this version to local tag pr-866/adlternative/gitk_tag_new_opt-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-866/adlternative/gitk_tag_new_opt-v2

@kxrob
Copy link

kxrob commented Feb 17, 2021

Noticed another issue: After tag remove, errors when clicking elsewhere. This requires some cache clearing (new proc removedtag). Also inserted a confirm dialog for tag remove. and dots in names for commands with dialog. Patch:

--- gitk
+++ gitk
@@ -2719,7 +2719,7 @@
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
         {mc "Check out this branch" command cobranch}
-        {mc "Rename this branch" command mvbranch}
+        {mc "Rename this branch..." command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
@@ -2727,8 +2727,8 @@
 
     set tagctxmenu .tagctxmenu
     makemenu $tagctxmenu {
-        {mc "Rename this tag" command mvtag}
-        {mc "Remove this tag" command rmtag}
+        {mc "Rename this tag..." command mvtag}
+        {mc "Remove this tag..." command rmtag}
         {mc "Copy tag name" command {clipboard clear; clipboard append $tagmenutag}}
     }
     $tagctxmenu configure -tearoff 0
@@ -9714,10 +9714,11 @@
     } else {
         notbusy renametag
         removetag $id $prevname
+        removedtag $id $tag
         set tagids($name) $id
         lappend idtags($id) $name
-        redrawtags $id
         addedtag $id
+        redrawtags $id
         dispneartags 0
         run refill_reflist
     }
@@ -10179,6 +10180,7 @@
     set tag $tagmenutag
     set id $tagmenuid
 
+    if {![confirm_popup [mc "Really delete tag %s?" $tag]]} return
     nowbusy rmtag
     update
     if {[catch {exec git tag -d $tag} err]} {
@@ -10187,8 +10189,10 @@
         return
     }
     removetag $id $tag
+    removedtag $id $tag
     redrawtags $id
     notbusy rmtag
+    dispneartags 0
     run refill_reflist
 }
 
@@ -11407,6 +11411,14 @@
     unset -nocomplain cached_atags
 }
 
+proc removedtag {id tag} {
+    global cached_dtags cached_atags cached_tagcontent
+
+    unset -nocomplain cached_tagcontent
+    unset -nocomplain cached_dtags 
+    unset -nocomplain cached_atags
+}
+
 proc addedhead {hid head} {
     global arcnos arcout cached_dheads
 

gitk-tagmenu2.patch.txt

@adlternative
Copy link
Author

Noticed another issue: After tag remove, errors when clicking elsewhere. This requires some cache clearing (new proc removedtag). Also inserted a confirm dialog for tag remove. and dots in names for commands with dialog. Patch:

--- gitk
+++ gitk
@@ -2719,7 +2719,7 @@
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
         {mc "Check out this branch" command cobranch}
-        {mc "Rename this branch" command mvbranch}
+        {mc "Rename this branch..." command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
@@ -2727,8 +2727,8 @@
 
     set tagctxmenu .tagctxmenu
     makemenu $tagctxmenu {
-        {mc "Rename this tag" command mvtag}
-        {mc "Remove this tag" command rmtag}
+        {mc "Rename this tag..." command mvtag}
+        {mc "Remove this tag..." command rmtag}
         {mc "Copy tag name" command {clipboard clear; clipboard append $tagmenutag}}
     }
     $tagctxmenu configure -tearoff 0
@@ -9714,10 +9714,11 @@
     } else {
         notbusy renametag
         removetag $id $prevname
+        removedtag $id $tag
         set tagids($name) $id
         lappend idtags($id) $name
-        redrawtags $id
         addedtag $id
+        redrawtags $id
         dispneartags 0
         run refill_reflist
     }
@@ -10179,6 +10180,7 @@
     set tag $tagmenutag
     set id $tagmenuid
 
+    if {![confirm_popup [mc "Really delete tag %s?" $tag]]} return
     nowbusy rmtag
     update
     if {[catch {exec git tag -d $tag} err]} {
@@ -10187,8 +10189,10 @@
         return
     }
     removetag $id $tag
+    removedtag $id $tag
     redrawtags $id
     notbusy rmtag
+    dispneartags 0
     run refill_reflist
 }
 
@@ -11407,6 +11411,14 @@
     unset -nocomplain cached_atags
 }
 
+proc removedtag {id tag} {
+    global cached_dtags cached_atags cached_tagcontent
+
+    unset -nocomplain cached_tagcontent
+    unset -nocomplain cached_dtags 
+    unset -nocomplain cached_atags
+}
+
 proc addedhead {hid head} {
     global arcnos arcout cached_dheads
 

gitk-tagmenu2.patch.txt

Thanks for pointing me out the error, I really hope that some of these small problems can be avoided.

@adlternative
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 18, 2021

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-866/adlternative/gitk_tag_new_opt-v3

To fetch this version to local tag pr-866/adlternative/gitk_tag_new_opt-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-866/adlternative/gitk_tag_new_opt-v3

@adlternative
Copy link
Author

@kxrob Hi, I don't know if my patch is good? For the time being, I didn't see any reply in the mailing list.

Copy link

@kxrob kxrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be necessary to get the the menu for up to maxtags (=3) tags including:

gitk-git/gitk Outdated
@@ -6678,6 +6703,9 @@ proc drawtags {id x xt y1} {
-font $font -tags [list tag.$id text]]
if {$ntags >= 0} {
$canv bind $t <1> $tagclick
if {$ntags_copy < $maxtags} {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be necessary to get the the menu for up to maxtags (=3) tags including:

--- gitk
+++ gitk
@@ -6728,7 +6728,7 @@
                    -font $font -tags [list tag.$id text]]
         if {$ntags >= 0} {
             $canv bind $t <1> $tagclick
-            if {$ntags_copy < $maxtags} {
+            if {$ntags_copy <= $maxtags} {
               $canv bind $t $ctxbut [list tagmenu %X %Y $id $tag_quoted]
             }
         } elseif {$nheads >= 0} {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this function is indeed applicable to <=3 tags, instead of <3 tags.

@adlternative
Copy link
Author

@kxrob,
In addition, I also found a problem:
(e.g. we have three tags: v1 v2 v3, under normal circumstances it can
indeed display <= 3 tags.But when a commit contains multiple tags, branches,
and their length is too long, it will not display v1 v2 v3 respectively but will display
tag(s)... In this case, we want to avoid providing users with multiple options for tag
operations. )

So The solution is :

--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6617,8 +6617,8 @@ proc drawtags {id x xt y1} {
         set marks $idtags($id)
         set ntags [llength $marks]
         set ntags_copy $ntags
-        if {$ntags > $maxtags ||
-            [totalwidth $marks mainfont $extra] > $maxwidth} {
+        set out_of_bounds [expr {[totalwidth $marks mainfont $extra] > $maxwidth}]
+        if {$ntags > $maxtags || $out_of_bounds} {
             # show just a single "n tags..." tag
             set singletag 1
             if {$ntags == 1} {
@@ -6703,7 +6703,7 @@ proc drawtags {id x xt y1} {
                    -font $font -tags [list tag.$id text]]
         if {$ntags >= 0} {
             $canv bind $t <1> $tagclick
-            if {$ntags_copy < $maxtags} {
+            if {$ntags_copy <= $maxtags && !$out_of_bounds} {
               $canv bind $t $ctxbut [list tagmenu %X %Y $id $tag_quoted]
             }
         } elseif {$nheads >= 0} {

@adlternative
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2021

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-866/adlternative/gitk_tag_new_opt-v4

To fetch this version to local tag pr-866/adlternative/gitk_tag_new_opt-v4:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-866/adlternative/gitk_tag_new_opt-v4

@kxrob
Copy link

kxrob commented Feb 24, 2021

Here another bug fix :) , error after rename.
Seems, when I introduced the removedtag cache clear for rmtag, I copied it to mvtaggo without testing.

--- gitk
+++ gitk
@@ -9688,7 +9688,7 @@
     } else {
         notbusy renametag
         removetag $id $prevname
-        removedtag $id $tag
+        removedtag $id $prevname
         set tagids($name) $id
         lappend idtags($id) $name
         addedtag $id

@kxrob Hi, I don't know if my patch is good? For the time being, I didn't see any reply in the mailing list.

I think it is rather stable then. Maybe the commit msg could be condensed somewhat, perhaps like

gitk: Add right-click context menu for tags

Adds a context menu for tag icons with commands similar to what 
exits for branches: "Rename this tag", "Remove this tag", 
"Copy tag name".

, and also cc it to the main author @paulusmack .
Cc: Paul Mackerras [email protected]

I can add a review-approved in github before next submit.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2021

There is an issue in commit 19fd0ef:
Prefixed commit message must be in lower case: gitk: Add right-click context menu for tags

Adds a context menu for tag icons with commands similar to what
exits for branches: "Rename this tag", "Remove this tag",
"Copy tag name".

Signed-off-by: ZheNing Hu <[email protected]>
@adlternative adlternative changed the title gitk: tag add right click options gitk: Add right-click context menu for tags Feb 24, 2021
@adlternative adlternative changed the title gitk: Add right-click context menu for tags gitk: add right-click context menu for tags Feb 24, 2021
Copy link

@kxrob kxrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works here. (Co-edited and made original feature request.)

@adlternative
Copy link
Author

adlternative commented Feb 24, 2021

Works here. (Co-edited and made original feature request.)
@kxrob Sorry for some misunderstandings: if you mean that I need to add your signature to the commit message, I may need your email. :)

@kxrob
Copy link

kxrob commented Feb 24, 2021

@adlternative I think thats not necessary - just small edits. Lets see when and if they find time and maybe have other requests. Seems, only few, mainly the main author, are involved in gitk / TCL. Maybe also write in the top post (for the letter), that its similar to what exists for branches. And that it is now likely consolidated. (The limit to 3 tags / out of bounds is obvious due to the preexisting logic.)

@adlternative
Copy link
Author

@adlternative I think thats not necessary - just small edits. Lets see when and if they find time and maybe have other requests. Seems, only few, mainly the main author, are involved in gitk / TCL. Maybe also write in the top post (for the letter), that its similar to what exists for branches. And that it is now likely consolidated. (The limit to 3 tags / out of bounds is obvious due to the preexisting logic.)

Okay, thanks for your help. I will send this patch.

@adlternative
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 25, 2021

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-866/adlternative/gitk_tag_new_opt-v5

To fetch this version to local tag pr-866/adlternative/gitk_tag_new_opt-v5:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-866/adlternative/gitk_tag_new_opt-v5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants