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