Tk Source Code

View Ticket
Bounty program for improvements to Tcl and certain Tcl packages.
Ticket UUID: 3f456a5bb980e9db5e5dd39db2f40b8021fb428
Title: Patches for listbox right justify
Type: Patch Version: 8.6
Submitter: tombert Created on: 2014-01-27 18:16:29
Subsystem: 09. [listbox] Assigned To: fvogel
Priority: 6 Severity: Minor
Status: Closed Last Modified: 2016-01-25 20:17:37
Resolution: Fixed Closed By: fvogel
    Closed on: 2016-01-25 20:17:37
I created patches for listbox right justify (reused patches from 8.4):
User Comments: fvogel added on 2016-01-25 20:17:37:
TIP #441 was accepted by TCT vote, and the corresponding implementation was merged to trunk.

Closing now.

fvogel added on 2016-01-19 07:13:26:
TIP #441 was published (thanks to the editor).

The feature branch hosting the implementation of it got renamed from rfe-3f456a5bb9 to tip-441.

fvogel added on 2016-01-18 21:05:25:
All the below issues/questions now sorted out I think.

I have written a TIP, I'm now waiting for it to be published.

fvogel added on 2016-01-12 15:25:45:
Here is my current (and certainly non-exhaustive) list of points to address regarding the patch:

Issue A:

package req Tk
listbox .l
.l insert end M M M M M M M M M
pack .l
.l conf -just center   ; # or right
# the selected item (not the active one) is not aligned with
# the other items (it is slightly shifted to the left)
# this slight shift does not depend on the width of the listbox
# it's however OK with -justify left

 . problem gets worse when increasing -borderwidth
   nullifies (or...? not sure...) when -bd 0
   --> -borderwidth was forgotten in the patch
 . ditto: -highlightthickness forgotten in the patch
 . but OK for -selectborderwidth
Testcase added.

Issue B:

Dependency of the bbox results on -justify were forgotten in the patch.
Testcase added.


1. Comments for xOffset declaration: are they still valid?

2. Shouldn't the code at lines 583+:
    if (listPtr->justify == TK_JUSTIFY_RIGHT) {
        listPtr->xOffset = GetMaxOffset(listPtr);
    } else if (listPtr->justify == TK_JUSTIFY_CENTER) {
        listPtr->xOffset = GetMaxOffset(listPtr) / 2;
        listPtr->xOffset -= listPtr->xOffset % listPtr->xScrollUnit;
be in ConfigureListbox rather than in Tk_ListboxObjCmd?
Shouldn't xOffset be updated when configuring the listbox (.l configure...)?

3. Analyze whether the oldoffset/tmpoffset stuff in ListboxEventProc is really needed?

4. Why using a GetMaxOffset procedure instead of caching the value in the listbox record?

5. Is it really needed to cache oldMaxOffset in the widget record?

6. The other available patch ([454303]) uses Tk_TextWidth but not Tk_ComputeTextLayout. How does this compare, from a performance point of view in particular?

I will now address all the above point one by one, and probably add a few new points to that list before the TIP phase can be launched.

fvogel added on 2016-01-11 17:31:27:
I had a first look at the patch.

I need to spend a bit more time to better understand some of the things it does.

Also, I have found at least one issue regarding alignment of listbox items.

Stay tuned, I'm working on this.

fvogel added on 2016-01-10 19:04:36:

I have planned to address this, yes.

But not now.

Besides, we should have a look to another ticket: [454303], and decide which patch is best, the one the present RFE or [454303], or if they are in fine from the same origin.

If you want to proceed, feel free of course.

jan.nijtmans added on 2016-01-10 14:26:58:

This RFE would also benefit from a quick TIP/voting. Opinions? Probably won't make 8.6.5, but 8.6.6 should be very well possible. Implementation is done already in branch "rfe-3f456a5bb9", just missing some test-cases

dkf added on 2014-02-11 15:13:17:

They look fine. Will need a minor TIP but that shouldn't be a problem at all, and won't need any fuss over who writes it (it's about a one-page document for something this small, really just to explain why we would want it to our future selves).

Also needs documentation and tests before we can go final.

jan.nijtmans added on 2014-02-11 14:54:57:


dkf added on 2014-01-27 21:18:46:

I suppose the next thing to do would be to get the patches instantiated as a fossil branch.