Skip to content

gitk: Tcl/Tk 9.0 support #11

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

qykth-git
Copy link
Contributor

This PR is for Tcl/Tk 9.0 support.
See also #5 .

It includes:

  • Tcl 9.0 fix
  • Tk 9.0 fix for mouse wheel code
  • Bug fix for combo box on preferences dialog
  • etc.

This PR works at least my Linux box.
So, you might needs some more test on other platforms.

@j6t
Copy link
Owner

j6t commented May 6, 2025

Thank you very much!

Please modify the patch series in the following way:

  • Sign off the commits.
  • Please do not make subject lines overly long. 72 characters or so should be the limit.
  • Do not make follow-up commits that fix things that went wrong or suboptimal in earlier commits. Pretend that you had perfect foresight and were able to come up with the optimal change in each commit from the get go.
  • Please do not make cosmetic fixes in places that are not otherwise touched. There might be exceptions to this rule if in the result an unchanged line would stick out like a sore thumb. But these cases are rare.

@qykth-git
Copy link
Contributor Author

@j6t
Thank you for your advice.
I was updated my PR.

@j6t
Copy link
Owner

j6t commented May 8, 2025

Thanks.

The last commit is questionable. We do have comboboxes elsewhere in the code, and it looks like there is proc makedroplist that is used to construct them. Can we use that here, too?

@qykth-git qykth-git force-pushed the tcltk-9.0 branch 3 times, most recently from 0e41210 to c259b2e Compare May 8, 2025 12:59
@qykth-git
Copy link
Contributor Author

@j6t
Thank you.

I use proc makedroplist to build combobox menus.

@j6t
Copy link
Owner

j6t commented May 8, 2025

Thank you, much appreciated. Queued.

@j6t
Copy link
Owner

j6t commented May 10, 2025

I am sorry, I changed my mind. The new plan is to lift the version requirement to 8.6 first, and then reconsider this PR.

@qykth-git
Copy link
Contributor Author

@j6t
Ok.
If something happens when marge this PR, please tell me.

@mark987
Copy link
Contributor

mark987 commented May 10, 2025

@j6t - I thought the gitk workflow uses the git email workflow on the git list. I have not seen any of these patches there, was surprised to see this here. Has something changed? Regardless, I've been working this too.

Since commit 904b36b, gitk requires Tk >= 8.5, and requires themed widgets. the problem is direct use of ${NS}::combobox rather than proc makedroplist.

mouse wheel handling was changed by Tcl's TIP 474 to unify events on all platforms. proc windows_mousewheel_redirector already handles this, so a one-line patch to invoke that routine for all platforms on TK9.0+ is apparently sufficient (this does work for me). Much simpler than 073c5c7.

Tcl 9.0 is not ready for use: my patches, or the ones here which do roughly the same things, yield a gitk that crashes in Tcl on the git repo, at least with the tcltk in Fedora 42. There is certainly no hurry to get this done.

@j6t
Copy link
Owner

j6t commented May 10, 2025

All good feedback, thank you!

Once you are on Github, you have to adapt. While I dislike Github's review system, distributing patches via email is so... 90s. Forcing fresh contributors this route doesn't work well. They are used to the shiny Web UIs...

@qykth-git
Copy link
Contributor Author

@mark987
Thank you for your suggest.

I was update this PR with TIP 474 for uniform mouse wheel handler.

@qykth-git
Copy link
Contributor Author

@j6t
I was added TIP 474 based mouse wheel code at 45d7aa8.

TIP 474: Treat the mouse wheel events in a uniform way
https://core.tcl-lang.org/tips/doc/trunk/tip/474.md
On macOS, the scaling for MouseWheel buttons has always been different from those on Windows, with a factor of 120. This means that applications assuming Tk 8.6 <MouseWheel> events will need to be modified to the new behaviour, otherwise the screen movements will be far too much.

It might helps macOS.

@j6t
Copy link
Owner

j6t commented May 25, 2025

Is there there an advantage in having the last patch 45d7aa8 (TIP474 mouse wheel support) in addition to 073c5c7 (mouse wheel support)? Can we not squash these two together and call the result "TIP474 mouse wheel support"?

@qykth-git
Copy link
Contributor Author

@j6t
Yes, they can be squash to one patch.
I was squashed the mouse wheel patches to one.

@mark987
Copy link
Contributor

mark987 commented May 27, 2025

I've submitted a pull request with my version of updates for 8.6 / 9.0. This is the best way I see to expose my findings using github, I expect we'll work together to get the best result, there is much in common but I do have some important differences that need inclusion.

qykth-git added 5 commits May 30, 2025 22:29
Signed-off-by: YOKOTA Hiroshi <[email protected]>
> unknown encoding "binary": No longer supported.
> please use either "-translation binary" or "-encoding iso8859-1"

Signed-off-by: YOKOTA Hiroshi <[email protected]>
"fconfigure" is obsoleted since Tcl 9 and superseded by "chan configure".
"chan configure" is also works on Tcl 8.6.

Signed-off-by: YOKOTA Hiroshi <[email protected]>
TIP 474 provides uniform way to handle mouse wheel events for all
platforms.

> TIP 474: Treat the mouse wheel events in a uniform way
> https://core.tcl-lang.org/tips/doc/trunk/tip/474.md

TIP 474 was available since Tk 8.7 for development version,
and also available since Tk 9.0 for release version.

So, use TIP 474 based mouse wheel event handler for all Tk 9 based
platforms.

Also, adjust horizontal scroll size from 5 to 1 for human eyes.
5 is too big to follow eyes while scrolling.

Signed-off-by: YOKOTA Hiroshi <[email protected]>
@qykth-git
Copy link
Contributor Author

@j6t
I was update my PR to fix encoding error that reported in #20 .
These fixes are taken from #20 by @mark987 .

@j6t
Copy link
Owner

j6t commented Jun 9, 2025

Please let's pursue this topic in #20, where @mark987 provides many more details in the commit messages on what is going on.

BTW, just a reminder for future contributions: If you cherry-pick other contributors commits, you are expected to sign off the commit, too.

mark987 added 2 commits June 10, 2025 00:16
gitk invokes many git commands expecting output in utf-8 encoding, but
git accepts extended ascii (code page unknown) as utf-8 without
validating, so cannot guarantee valid utf-8 on output.  In particular,
using any extended ascii code page, of which there are many, has long
been acceptable given that everyone on a project is aware of and uses
that same code page to view all data. utf-8 accepts only 7-bit ascii
characters in single bytes, and any characters outside of that base set
require at least two bytes.

Tcl is a string based language, and transcodes all input data to an
internal unicode format, and to whatever format is requested on output:
"pure" binary is recoded using iso8859-1.  Tcl8.x silently recodes
invalid utf-8 as binary data, so extended ascii characters maintain
their binary value on output but may not display correctly.

Tcl 8.7 added three profiles to control this behviour: strict (raises
execptions), replace (replaces each invalid byte with ?), and the
default tcl8 maintaining the old behavior.  Tcl 9 changes the default
profile to strict, meaning any invalid utf-8 raises an exception that
gitk does not handle.

An example of this in the git repository is commit 7eb93c8965 ("[PATCH]
Simplify git script", 2005-09-07). This includes extended ascii
characters in the author name and commit message. As a result, gitk +
Tcl 9 cannot view the git repository at any point beyond that commit.
Note: Tcl 9.0 has a bug, to be fixed in 9.1, where this particular
condition results in a memory error causing Tcl to crash [1].

The tcl8 profile used so far has acceptable behavior given gitk's
acceptance: this allows gitk to accept extended ascii though it may
display incorrectly.  Let's continue that behavior by overriding open to
use the tcl8 profile on Tcl9 and later: Tcl 8.6 does not understand
fconfigure -profile, and Tcl 8.7 maintains the tcl8 profile.

[1] Per https://core.tcl-lang.org/tcl/tktview/73bb42fb3f35cd613af6fcea465e35bbfd352216

Signed-off-by: Mark Levedahl <[email protected]>
Signed-off-by: YOKOTA Hiroshi <[email protected]>
gitk in the prior commit learned to apply -profile tcl8 to all input
data streams, avoiding errors on non-binary data streams whose encoding
is not utf-8. But, gitk also consumes binary data streams (generally blobs
from commits), and internally decodes this to support various displays.

With Tcl9, errors occur in this decoding for the same reasons described
in the previous commit: basically, the underlying data was not validated
to conform to the given encoding, and this source encoding may not be
utf-8. gitk performs this decoding using Tcl's '[encoding convert from'
operator.

For example, the 7th commit in gitk's history has the extended ascii
value 0xA9, so

	gitk 9a40c50

in gitk's repository raises an exception. The error log has:

unexpected byte sequence starting at index 11: '\xA9'
    while executing
"encoding convertfrom $diffencoding $line"
    (procedure "parseblobdiffline" line 135)
    invoked from within
"parseblobdiffline $ids $line"
    (procedure "getblobdiffline" line 16)
    invoked from within
"getblobdiffline file6 9a40c50"
    ("eval" body line 1)
    invoked from within
"eval $script"
    (procedure "dorunq" line 11)
    invoked from within
"dorunq"
    ("after" script)

This problem has a similar fix to the prior issue: we must use the tlc8
profile when converting this data. Do so, again only on Tcl9 as Tcl8.6
does not recoginize -profile, and only Tcl 9.0 makes strict the default.

Signed-off-by: Mark Levedahl <[email protected]>
Signed-off-by: YOKOTA Hiroshi <[email protected]>
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.

3 participants