gitk lower pane (commit and files view) scrollbar extends past gitk window

gitk lower pane (commit and files view) scrollbar extends past gitk window

[Jakub Narebski]

In gitk from the current 'next' branch, post git version 1.4.0 
(blob ba4644f) scrollbar for lower pane, i.e. for commitdiff and files
(Comments) views extends past the bottom of the gitk window. Therefore 
I cannnot see lower part of commit diff if it is larger than window height.

gitk window has height lower than 700.

The upper pane, i.e. history graph scrollbar is correct, and works as it
should.

This is regression from git 1.3.0, where gitk works correctly.


Below ascii art trying to describe situation.

  _                                _                                  _
  ^  .. upper scrollbar arrow .... ^                                  ^
  #                                #                                  |
  #  .... position indicator ..... #                                  |
  |                                |                                  |
  |                                |    scrolling past window edge -> #
  |      bottom of gitk window --> =                                  =
  v                                  <-- no lower scrollbar arrow
  =  <-- bottom of gitk window

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: gitk lower pane (commit and files view) scrollbar extends past gitk window

[Edgar Toernig]

Jakub Narebski wrote:
>
> In gitk from the current 'next' branch, post git version 1.4.0 
> (blob ba4644f) scrollbar for lower pane, i.e. for commitdiff and files
> (Comments) views extends past the bottom of the gitk window. Therefore 
> I cannnot see lower part of commit diff if it is larger than window height.

Yes, and the search field at the bottom is invisible too.  Removing
line 431:

        .ctop conf -width $geometry(width) -height $geometry(height)

seems to fix it and the window still gets the right size.

Ciao, ET.

[PATCH] gitweb: whitespace cleanup around '='

[Jakub Narebski]

Signed-off-by: Jakub Narebski <jnareb@gmail.com>


---

 gitweb/gitweb.cgi |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

82c8812870e9c40fd495e9c3e2eae0eb78ac2759
diff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi
index e044c61..adc4a42 100755
--- a/gitweb/gitweb.cgi
+++ b/gitweb/gitweb.cgi
@@ -326,7 +326,7 @@ EOF
 		} elsif (defined $hash) {
 			$search_hash = $hash;
 		} else {
-			$search_hash  = "HEAD";
+			$search_hash = "HEAD";
 		}
 		$cgi->param("a", "search");
 		$cgi->param("h", $search_hash);
@@ -1766,8 +1766,8 @@ sub git_opml {
 		}
 
 		my $path = esc_html(chop_str($proj{'path'}, 25, 5));
-		my $rss =  "$my_url?p=$proj{'path'};a=rss";
-		my $html =  "$my_url?p=$proj{'path'};a=summary";
+		my $rss  = "$my_url?p=$proj{'path'};a=rss";
+		my $html = "$my_url?p=$proj{'path'};a=summary";
 		print "<outline type=\"rss\" text=\"$path\" title=\"$path\" xmlUrl=\"$rss\" htmlUrl=\"$html\"/>\n";
 	}
 	print "</outline>\n".
@@ -1942,7 +1942,7 @@ sub git_commit {
 	      "<td class=\"link\">" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$hash")}, "tree") .
 	      "</td>" .
 	      "</tr>\n";
-	my $parents  = $co{'parents'};
+	my $parents = $co{'parents'};
 	foreach my $par (@$parents) {
 		print "<tr>" .
 		      "<td>parent</td>" .
-- 
1.3.0

Re: [PATCH] gitweb: whitespace cleanup around '='

[Jakub Narebski]

I'm sorry, wrong In-Reply-To given to git-send-email.
It should be reply to <7vu06exfym.fsf@assigned-by-dhcp.cox.net>

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: whitespace cleanup around '='

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> @@ -326,7 +326,7 @@ EOF
>  		} elsif (defined $hash) {
>  			$search_hash = $hash;
>  		} else {
> -			$search_hash  = "HEAD";
> +			$search_hash = "HEAD";
>  		}
>  		$cgi->param("a", "search");
>  		$cgi->param("h", $search_hash);

My copy from "next" has this part like the following:

		if (!defined $searchtext) {
			$searchtext = "";
		}
		my $search_hash;
		if (defined $hash) {
			$search_hash = $hash;
		} else {
			$search_hash  = "HEAD";
		}

I thought we were both reasonably sure that we are now in
sync...  I could apply it by hand but once I start doing that
I'm afraid we will never converge.

It's quite frustrating.

> @@ -1766,8 +1766,8 @@ sub git_opml {
>  		}
>  
>  		my $path = esc_html(chop_str($proj{'path'}, 25, 5));
> -		my $rss =  "$my_url?p=$proj{'path'};a=rss";
> -		my $html =  "$my_url?p=$proj{'path'};a=summary";
> +		my $rss  = "$my_url?p=$proj{'path'};a=rss";
> +		my $html = "$my_url?p=$proj{'path'};a=summary";
>  		print "<outline type=\"rss\" text=\"$path\" title=\"$path\" xmlUrl=\"$rss\" htmlUrl=\"$html\"/>\n";
>  	}
>  	print "</outline>\n".

Are you sure you want to do "my $rss two-spaces equal space"?

I've fixed up the whitespace you missed and will be applying the
following on top of what is in "next" tonight.

-- >8 --
gitweb: do not align assignment = with extra whitespaces

---
diff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi
index f2e5046..e2ed6db 100755
--- a/gitweb/gitweb.cgi
+++ b/gitweb/gitweb.cgi
@@ -55,7 +55,7 @@ my $projects_list = "index/index.aux";
 
 # default blob_plain mimetype and default charset for text/plain blob
 my $default_blob_plain_mimetype = 'text/plain';
-my $default_text_plain_charset  = undef;
+my $default_text_plain_charset = undef;
 
 # file to use for guessing MIME types before trying /etc/mime.types
 # (relative to the current git repository)
@@ -325,7 +325,7 @@ EOF
 		if (defined $hash) {
 			$search_hash = $hash;
 		} else {
-			$search_hash  = "HEAD";
+			$search_hash = "HEAD";
 		}
 		$cgi->param("a", "search");
 		$cgi->param("h", $search_hash);
@@ -1283,21 +1283,21 @@ HTML
 
 		if ($line =~ m/^([0-9a-fA-F]{40})\t\(\s*([^\t]+)\t(\d+) \+\d\d\d\d\t(\d+)\)(.*)$/) {
 			$long_rev = $1;
-			$author   = $2;
-			$time     = $3;
-			$lineno   = $4;
-			$data     = $5;
+			$author = $2;
+			$time = $3;
+			$lineno = $4;
+			$data = $5;
 		} else {
 			print qq(  <tr><td colspan="5" class="error">Unable to parse: $line</td></tr>\n);
 			next;
 		}
-		$short_rev  = substr ($long_rev, 0, 8);
-		$age        = time () - $time;
-		$age_str    = age_string ($age);
-		$age_str    =~ s/ /&nbsp;/g;
-		$age_class  = age_class($age);
-		$author     = esc_html ($author);
-		$author     =~ s/ /&nbsp;/g;
+		$short_rev = substr ($long_rev, 0, 8);
+		$age = time () - $time;
+		$age_str = age_string ($age);
+		$age_str =~ s/ /&nbsp;/g;
+		$age_class = age_class($age);
+		$author = esc_html ($author);
+		$author =~ s/ /&nbsp;/g;
 		# escape tabs
 		while ((my $pos = index($data, "\t")) != -1) {
 			if (my $count = (8 - ($pos % 8))) {
@@ -1765,8 +1765,8 @@ sub git_opml {
 		}
 
 		my $path = esc_html(chop_str($proj{'path'}, 25, 5));
-		my $rss =  "$my_url?p=$proj{'path'};a=rss";
-		my $html =  "$my_url?p=$proj{'path'};a=summary";
+		my $rss = "$my_url?p=$proj{'path'};a=rss";
+		my $html = "$my_url?p=$proj{'path'};a=summary";
 		print "<outline type=\"rss\" text=\"$path\" title=\"$path\" xmlUrl=\"$rss\" htmlUrl=\"$html\"/>\n";
 	}
 	print "</outline>\n".
@@ -1941,7 +1941,7 @@ sub git_commit {
 	      "<td class=\"link\">" . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$hash")}, "tree") .
 	      "</td>" .
 	      "</tr>\n";
-	my $parents  = $co{'parents'};
+	my $parents = $co{'parents'};
 	foreach my $par (@$parents) {
 		print "<tr>" .
 		      "<td>parent</td>" .

Re: [PATCH] gitweb: whitespace cleanup around '='

[Jakub Narebski]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> @@ -326,7 +326,7 @@ EOF
>>              } elsif (defined $hash) {
>>                      $search_hash = $hash;
>>              } else {
>> -                    $search_hash  = "HEAD";
>> +                    $search_hash = "HEAD";
>>              }
>>              $cgi->param("a", "search");
>>              $cgi->param("h", $search_hash);
> 
> My copy from "next" has this part like the following:
> 
>               if (!defined $searchtext) {
>                       $searchtext = "";
>               }
>               my $search_hash;
>               if (defined $hash) {
>                       $search_hash = $hash;
>               } else {
>                       $search_hash  = "HEAD";
>               }
> 
> I thought we were both reasonably sure that we are now in
> sync...  I could apply it by hand but once I start doing that
> I'm afraid we will never converge.
> 
> It's quite frustrating.

I'm very sorry, I forgot that I applied Timo Hirvonen patch

  "[PATCH] gitweb: Start search from HEAD"
  (<20060620152515.23e59396.tihirvon@gmail.com>)
  http://permalink.gmane.org/gmane.comp.version-control.git/22197>

on top of 'next'.

This patch fixes search quite nicely, reducing significantly number of error
messages during search (e.g. starting search from blob or file view).

Any reason not to apply it?

>>              my $path = esc_html(chop_str($proj{'path'}, 25, 5));
>> -            my $rss =  "$my_url?p=$proj{'path'};a=rss";
>> -            my $html =  "$my_url?p=$proj{'path'};a=summary";
>> +            my $rss  = "$my_url?p=$proj{'path'};a=rss";
>> +            my $html = "$my_url?p=$proj{'path'};a=summary";
> 
> Are you sure you want to do "my $rss two-spaces equal space"?
> 
> I've fixed up the whitespace you missed and will be applying the
> following on top of what is in "next" tonight.
> 
> -- >8 --
> gitweb: do not align assignment = with extra whitespaces

My idea for coding style is to align assignment = with extra whitespaces,
but only where aligned assignments statements are one after another.

The above "my $rss" example was mistyped as " =  " instead of "  = ",
but I think it was meant to be aligned at '='. IMHO aligning at assign 
makes code more readable.

But that is just my opinion.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: whitespace cleanup around '='

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> I thought we were both reasonably sure that we are now in
>> sync...  I could apply it by hand but once I start doing that
>> I'm afraid we will never converge.
>> 
>> It's quite frustrating.
>
> I'm very sorry, I forgot that I applied Timo Hirvonen patch
>
>   "[PATCH] gitweb: Start search from HEAD"
>   (<20060620152515.23e59396.tihirvon@gmail.com>)
>   http://permalink.gmane.org/gmane.comp.version-control.git/22197>
>
> on top of 'next'.

I remember discussing things with Timo and was putting it on
hold since I knew you were also actively futzing on gitweb.

But I think what you have is this one instead:

Message-ID: <20060620164105.7276a45f.tihirvon@gmail.com>

I applied it in jn/gitweb topic and then applied your whitespace
fixes on top of the result, after discarding the whitespace
cleanup I sent out earlier.  So let's make sure we are resync'ed
and continue from here.  I'll push out the updated "next"
soonish.

Re: [PATCH] gitweb: whitespace cleanup around '='

[Jakub Narebski]

Junio C Hamano wrote:

> I remember discussing things with Timo and was putting it on
> hold since I knew you were also actively futzing on gitweb.

So when I apply someone else patches should I reply to the post
with patch I have applied with

  Acked-by: Jakub Narebski <jnareb@gmial.com>

to notify of that fact?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: whitespace cleanup around '='

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> I remember discussing things with Timo and was putting it on
>> hold since I knew you were also actively futzing on gitweb.
>
> So when I apply someone else patches should I reply to the post
> with patch I have applied with
>
>   Acked-by: Jakub Narebski <jnareb@gmial.com>
>
> to notify of that fact?

Acked-by does not mean you've applied it to your tree, and even
if it did, it is unfair to expect me to remember that fact when
I review your patch later.  In this particular case, a note in
the patch to say it depends on such and such that _I_ do not
seem to have yet would have been more appropriate.

Acked-by would also be helpful when a de-facto owner of the code
wants to push acceptance of others' patches, but that is
independent from stating what a patch's prerequisites are.

Anyhow, I've reviewed Timo's patch (which made sense to me too),
and your whitespace fixes and pushed out the result, which
hopefully will be propagating to the mirrors soon.  Overall, I
would say that the clean-up on gitweb so far is reasonably
straightforward.  I have been running "next" version of gitweb
on my private machine and haven't found regression.

I'll go to bed now.  Thanks for the patches.

Re: [PATCH] gitweb: whitespace cleanup around '='

[Jakub Narebski]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:

>> I'm very sorry, I forgot that I applied Timo Hirvonen patch
>>
>>   "[PATCH] gitweb: Start search from HEAD"
>>   Message-ID: <20060620152515.23e59396.tihirvon@gmail.com>
> 
> But I think what you have is this one instead:
> 
> Message-ID: <20060620164105.7276a45f.tihirvon@gmail.com>

Is the 'note' extension to commit header implemented yet? It it is,then it
would be nice if git-am recorded original email message-ids in the note
header. Usually it is of no interest to user, so it should be hidden by
default.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: whitespace cleanup around '='

[Santi Béjar]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> I'm very sorry, I forgot that I applied Timo Hirvonen patch
>>>
>>>   "[PATCH] gitweb: Start search from HEAD"
>>>   Message-ID: <20060620152515.23e59396.tihirvon@gmail.com>
>> 
>> But I think what you have is this one instead:
>> 
>> Message-ID: <20060620164105.7276a45f.tihirvon@gmail.com>
>
> Is the 'note' extension to commit header implemented yet? It it is,then it
> would be nice if git-am recorded original email message-ids in the note
> header. Usually it is of no interest to user, so it should be hidden by
> default.

I put it at the end of the commit message, like the Signed-of-by
lines. And, yes, it would be nice if git-am would add it (--message-id
flag?). If more people is interested I'll try to send a patch.

        Santi

Re: gitk lower pane (commit and files view) scrollbar extends past gitk window

[Alex Riesen]

On 6/22/06, Edgar Toernig <froese@gmx.de> wrote:
> Jakub Narebski wrote:
> >
> > In gitk from the current 'next' branch, post git version 1.4.0
> > (blob ba4644f) scrollbar for lower pane, i.e. for commitdiff and files
> > (Comments) views extends past the bottom of the gitk window. Therefore
> > I cannnot see lower part of commit diff if it is larger than window height.
>
> Yes, and the search field at the bottom is invisible too.  Removing
> line 431:
>
>         .ctop conf -width $geometry(width) -height $geometry(height)
>
> seems to fix it and the window still gets the right size.
>

No, it does not. It gets the default geometry, not the one it had last time.

[WORKAROUND] gitk lower pane scrollbar extends past gitk window

[Jakub Narebski]

Alex Riesen wrote:

> On 6/22/06, Edgar Toernig <froese@gmx.de> wrote:
>> Jakub Narebski wrote:
>>>
>>> In gitk from the current 'next' branch, post git version 1.4.0
>>> (blob ba4644f) scrollbar for lower pane, i.e. for commitdiff and files
>>> (Comments) views extends past the bottom of the gitk window. Therefore
>>> I cannnot see lower part of commit diff if it is larger than 
>>> window height. 
>>
>> Yes, and the search field at the bottom is invisible too.  Removing
>> line 431:
>>
>>         .ctop conf -width $geometry(width) -height $geometry(height)
>>
>> seems to fix it and the window still gets the right size.
>>
> 
> No, it does not. It gets the default geometry, not the one it had last time.

Now I see. It is the case when I used gitk from a computer with a desktop
with larger screen height, then on computer with screen height smaller than 
gitk height was I had mentioned problem.

Workaround for now is to delete old geometry info from ~/.gitk file 
(or just delete whole file).

I don't know Tcl/Tk to make proper fix, i.e. check if restored size is not
larger than available screen size, and use min(saved size, screen size).
Another improvement would be to save the fact that gitk window is maximized
as a symbol, e.g. as equivalent of 

  set geometry(height) screen(height)

in ~/.gitk

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [WORKAROUND] gitk lower pane scrollbar extends past gitk window

[Jakub Narebski]

It looks like gitk incorrectly saves the geometry in ~/.gitk
I have to remove it before running gitk (well, it would be 
enough to remove geometry section).

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git