Tk Source Code

View Ticket
Login
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
Description:
I created patches for listbox right justify (reused patches from 8.4):
https://bitbucket.org/tombert/tcltk/src/38880b7dbf1e/patches/tcltk86/?at=master
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

Analysis: 
 . 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.


Questions:
----------

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: (text/x-fossil-wiki)
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: (text/x-fossil-wiki)
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: (text/html)
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).<p>Also needs documentation and tests before we can go final.

jan.nijtmans added on 2014-02-11 14:54:57: (text/x-fossil-wiki)
[87d4c7788e]

dkf added on 2014-01-27 21:18:46: (text/html)
I suppose the next thing to do would be to get the patches instantiated as a fossil branch.

Attachments: