Tk Source Code

Tcl/Tk 8.6.11 fontchooser has still issues:
- The dialog shows a lot of duplicated font families.
- Locale change doesn't change all text in dialog correctly. Font style in the listbox stays with the old locale.
- The title of the dialog is not changing in case the locale is changed.
- The sample text is also defined inside the namespace definition. This should also moved to ::tk::fontchooser::Show to handle all msgcat in the same way.
- Confguration of -title does not take effect directly. The dialog has to be destroyed by using button Ok or Cancel and created again. Expected behaviour would be that title is changed directly when tk fontchooser configure -title newValue is done.
- If font entry or size entry is cleared Ok button becomes disabled. The Apply button is still usable (state = normal).
- The dialog shows only the buttons Ok and Cancel if -command is not configured (is empty).
	In case configured we have three buttons: Ok, Cancel and Apply.
	That makes no sense, button Ok and Cancel do the same in case no command (callback) is configured. That means Cancel would be enough.
	On the other side button Apply raise event <<TkFontchooserFontChanged>> which is needed to use the dialog with empty -command.
	
	In case the command is configured later, we will still have Ok and Cancel only. We still miss the function of Apply.
	If we start with configured command all three buttons are shown. If the command is then set to {} we have again not correct working button.
	In both cases the dialog should be able to show that state. That could be done by display allways all three buttons and activate/deactivate them (state normal and disabled) depending on the situation.

Merged in core-8-6-branch and trunk.

Thanks a lot, Holger!

Unless someone objects (please test!), I will merge this soon.

To test, just try the 'tk fontchooser' command on Linux.

I have now fixed the issue that was triggering errors in the fontchooser test suite, and the same fix removes the <i>can't read "S(nstate)": no such element in array</i> error. See [4f9c9834].

Next, when addressing issue #15 I have found that proc ::tk::fontchooser::actual contains a mistake. The following line always overwrites the font family that was requested by the user when configuring a font (for example):

<verbatim>
  set F(-family) [dict get [font actual [list serif -14 normal roman underline overstrike italic]] -family]
</verbatim>

Analyzing this ::tk::fontchooser::actual proc further, and realizing that this proc was trying to address issue #14, I came to the conclusion that it should be entirely removed and that we really should rely on the internal [font actual]. I have understood that you have introduced proc ::tk::fontchooser::actual to work around what you think is a bug in [font actual]. However I'm not convinced there is a bug in [font actual], and in any case, if there is such a bug we should characterize it and fix it in the Tk font core code instead of introducing wobbly workarounds like ::tk::fontchooser::actual. That's the reason for [894a7a81], which fixes #15 (and, yes, resurrects #14 which should be fixed differently if it gets proven #14 is a bug - this latter question deserves a specific ticket BTW).

At this stage, basic testing of the dialog does not show any remaining bug for me.

Sorry for the delay, here am I back!

I have had a look at the proposed changes.

First thing I note is that with the new fontchooser.tcl there are errors in the fontchooser.test test suite (there is no error with core-8-6-branch). This must be fixed either by correcting mistakes in the fontchooser.tcl script, or by changing the tests or their results if deemed erroneous.

I have fixed one such mistake in fontchooser.tcl in [b0b8d38892]. There are remaining errors reported by the test suite (specifically: fontchooser-4.[1-4] fail, on Linux at least). I'm attaching the log.

Then, new tests should probably be added to test and check regressions that could be brought by the changes from the present ticket.

Besides, other unexpected errors:
<verbatim>
% tk fontchooser show
% proc confproc {} {}
% tk fontchooser configure -command confproc
can't read "S(nstate)": no such element in array
</verbatim>

This is caused by an incomplete fix for #7. Some initialisation step must be missing somewhere (note that at this point no trace did fire yet). As a consequence I could not test #7 correctly so far, but I'm sure that at the moment the greying out of 'OK' and 'Apply' buttons is incorrect in some situations such as this one.

That said, I have checked that #8, #9, #10, #11, #12, #13 and #14 are all working for me. Well done!

And another issue:

#15: When the dialog is open, calling 'tk fon con -font Times' does not seem to change the font selected in the dialog. This is contrary to the manual. Also, setting -font to the empty string when the dialog is open does not do what is described in the manual (i.e. select no font).

This time the correct file went through, thank you. I have committed this new version in the bugfix branch [https://core.tcl-lang.org/tk/timeline?r=bug-f75190db19|bug-f75190db19].

I will analyze your second report in more details, look at the corresponding fixes and do some testing.

I'm not sure what happened. Today I load up both files again.

<verbatim>
> I made the changes based on https://core.tcl-lang.org/tk/file?name=library/fontchooser.tcl&ci=8c3097c915a2a26d.
> Provided as the whole file and as a patch.
</verbatim>

Sorry, did you attach the new version? It looks like you attached your original patch and file.

I agree with you it is clumsy to press two buttons. 
My intension of #7 was not to change the general behaviour of the requester.

After reading your answer and analysing code and documentation a bit deeper I found two other issues before I changed code for #7.

#8 One is that Ok miss creation of event <<TkFontchooserFontChanged>>. The documentation writes about it: 
"Where an Apply or OK button is present in the dialog, that button will trigger the -command callback and <<TkFontchooserFontChanged>> virtual event."
Fix: Add event creation in ::tk::fontchooser::Done 

#9 The other one is that -font never is updated by selecting/changing the font. Here the documentation writes:
"Binding scripts can determine the currently selected font by querying the -font configuration option."
Fix: Add "set S(-font) $S(result)" at the end of ::tk::fontchooser::Update

#7 Now we can change the fix. Replace in ::tk::fontchooser::Configure and ::tk::fontchooser::Tracer
        if {$S(-command) eq {}} {
            $S(W).ok configure -state disabled
            $S(W).apply configure -state $S(nstate)
        } else {
            $S(W).ok configure -state $S(nstate)
            $S(W).apply configure -state $S(nstate)
        }
with
	    $S(W).ok configure -state $S(nstate)
        $S(W).apply configure -state $S(nstate)

During working this out I found by chance that we have trouble with the font size entry, too.

10th Non integer numbers as size like "8.2" are detected as wrong values but button 'Ok' and 'Cancel' still be enabled.
11th Entry field of size does not work properly with negativ values and 0.
Command font accepts as size signed integer. This means also negativ values, even 0, are allowed.
An example is "font configure TkCaptionFont -size" delivers -14.

One reason for 10th and 11th is the use of "string is double" which is not exactly a unsigned integer.
Fix: Replace in ::tk::fontchooser::Create the part "-validatecommand {string is double %P}" with "-validatecommand {regexp -- {^-*[0-9]*$} %P}"
And replace in ::tk::fontchooser::Tracer "[string is double -strict $value]" with "[regexp -- {^(-[0-9]+|[0-9]+)$} $value]"

12th An empty entry field size lead to:
	Error: expected integer but got ""
in case one is selecting a value from the font or font style list box.
Reason is that ::tk::fontchooser::Click calls ::tk::fontchooser::Update without checking the values.
This is not needed because the traces are fired and call ::tk::fontchooser::Tracer. 
Which then, if needed, calls ::tk::fontchooser::Update.
This also reduces the huge ammount of fired traces. 

13# Problem now is that Strikeout and Underline is not processed correctly.
Reason here is that these values are not monitored by traces like the others.
Fix: Add traces in ::tk::fontchooser::Create.
Add removing traces in::tk::fontchooser::Done
Change ::tk::fontchooser::Tracer to handle these new traces

Also the statement inside the code of ::tk::fontchooser::Tracer:
            # Size is weird: valid numbers are legal but don't display
            # unless in the font size list
is wrong from my point of view. All legal values have an effect in the sample label 
when "set bad 1" is defined at the correct place.
From before "if {$var ne "size" ..." inside the if statement.

13th Looks like size has a maximum number on X11. Size 4500 raised a X11 problem on my system, 4096 doesn't. 
Therefore, size should be limited. Perhaps this is not the best value.
In ::tk::fontchooser::Tracer "[regexp -- {^(-[0-9]+|[0-9]+)$} $value]" should be changed to "!([regexp -- {^(-[0-9]+|[0-9]+)$} $value] && $value >= -4096 && $value <= 4096)"

14th In case we configure -font with a font containing a negative size it is converted to a positive integer.
Reason is the usage of "font actual" to assure that the values are legal.
This converts negativ size to a positiv size. The issue here is that the results are different.
An example:
"font actual {DejaVu Sans} -14}" returns "-family {DejaVu Sans} -size 11 -weight normal -slant roman -underline 0 -overstrike 0"
If we try size -14 and size 11 with the example label we see a difference.
On the other side "font actual" converts -family to an existing family.

My opinion is that "font actual" has an issue in Linux. But, I can't proove it.
Therefore, I tried to work around it as good as possible. For this I introduced ::tk::fontchooser::actual as long as 'font actual' change size.
This will not preserve a negative/pixel size in every case but, better as nothing.

I made the changes based on https://core.tcl-lang.org/tk/file?name=library/fontchooser.tcl&ci=8c3097c915a2a26d.
Hope, that this is the latest one.
Provided as the whole file and as a patch.

That should remove the clumsy usage and fix the issues.

Thank you for such a high quality report. Your description of the problems and the chages needed are perfect. Many thanks, this is really clear.

I have analyzed your report, have reproduced all 7 issues you describe, and have checked that they no longer happen with your patched version of fontchooser.tcl.

I have merged your version with the current fontchooser.tcl (one change went in since you proposed your patch, see [6e449c0b]). The only line of your patch I didn't merge was the following line, which is a leftover debug statement I think:

<verbatim>
bind $S(W) <<TkFontchooserFontChanged>> [list puts {Font changed}]
</verbatim>

This merge constitutes commit [026142fc] in the bugfix branch [https://core.tcl-lang.org/tk/timeline?r=bug-f75190db19|bug-f75190db19].

I fully agree with all your changes, except the last one (#7) for which I'm wondering. Your rationale makes sense: "button Ok and Cancel do the same in case no command (callback) is configured. That means Cancel would be enough". However, I believe this is a programmer point of view. From the user point of view I find a bit disturbing that OK is not available when -command is {}. I view the OK action as being exactly the same as "Apply" except that the dialog gets closed. With the current patch the user needs to Apply and click a second time (on Cancel) to close the dialog, which I find a bit clumsy.

What do you think?