| Ticket UUID: | d82fa2953a93012806f0effea01701c68860b2 | |||
| Title: | Cosmetic issues including wrongly clipped items when ttk::treeview height is not a full number of lines. | |||
| Type: | Bug | Version: | fe177b0799 | |
| Submitter: | sbron | Created on: | 2024-08-15 10:37:48 | |
| Subsystem: | 88. Themed Tk | Assigned To: | sbron | |
| Priority: | 5 Medium | Severity: | Minor | |
| Status: | Closed | Last Modified: | 2024-08-29 18:28:11 | |
| Resolution: | Fixed | Closed By: | fvogel | |
| Closed on: | 2024-08-29 18:28:11 | |||
| Description: |
There are a number of issues with the ttk::treeview widget when the height of the widget doesn't correspond with an exact number of rows:
I also noticed that the 'see' command does not always work correctly when used immediately after the 'yview' command was used to change the view. As at least some of these issues are probably related to ticket [a9e637f1], I will look into it. | |||
| User Comments: |
fvogel added on 2024-08-29 18:28:11:
Closing since this got merged. Thanks to all involved! Follow-up to [8e9d65d2d4] for the latest problems identified by sbron. nemethi (claiming to be Csaba Nemethi) added on 2024-08-28 10:38:09: Schelte, I fully agree with your proposal to merge the changes in their current state and then open a new ticket for the problems related to detached items. sbron added on 2024-08-28 10:07:29: While looking into merging the branch and propagating the changes to Tk 8.7 and 9.0, I ran into some bugs in the TIP #552 implementation. The problems are related to detached items: Executing the `see` command for a detached item will scroll to the row where the item happened to be before it was detached. The `bbox` command returns information about the row where the item was before it was detached. Unfortunately, these problems can't be fully fixed by using IsDetached() because of the objection I made against TIP #678: IsDetached() will only return true for the root of a detached tree, not for the child items that disappeared along with their parent. Sadly my objections were completely ignored. I don't think it will be appropriate to attempt to fix the problems as part of the merge. I propose to just merge the changes in their current state and then open a new ticket for the bugs I encountered in Tk 8.7/9.0. jan.nijtmans added on 2024-08-27 22:30:02: "bug-d82fa2953a" branch looks good to me. I tested it on MacOS 12.5 Monterey, with the attached "demo.tcl" chw added on 2024-08-27 08:50:03: Schelte, well, then let's further shed our bike, see the new ticket title. sbron added on 2024-08-27 08:30:49: I have incorporated the changes into the "bug-d82fa2953a" branch. I'm less happy with the change of the ticket title. The ticket addressed several issues with the appearance and behavior of the ttk::treeview widget. The fact that one of those issues took much more effort to fix than the others doesn't mean it was the only issue, or even the most important one (to me). nemethi (claiming to be Csaba Nemethi) added on 2024-08-27 08:13:24: Harald, we are already in the branch "bug-d82fa2953a". IMHO, now it is Schelte's turn to check in the updated version of the attachment, provided that he finds it ripe for commit. As far as I can tell, the aqua-specific stuff now in a good enough shape, hence we should proceed with commit and merge. oehhar added on 2024-08-27 06:50:11: Great work! Great team! Shall I put the attachement into a branch? Harald nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 19:10:19: I have updated the attachment ttkTreeview.c. The TreeviewDisplay() function now properly checks whether the current theme is aqua and the OS version is > 10.8. If both conditions are fulfilled then it decrements y by 4 and increments height by the same amount. Instead of tkMacOSXInt.h, now tkMacOSXPrivate.h is included, which is needed for Tk 8.7 and 9, too. chw added on 2024-08-26 17:36:59: I'm quite happy that this issue is now going to get fixed with thorough oversight and competence. Let me therefore change the title of the ticket. IMO those are the lessons we all have learned: * ttk draws things in the order specified by layouts. The treeview defines the toplevel element to be the entire window with (optional) focus ring, relief, and client area. * The client area is then split in parcels for the heading and the tree items. * When the treeview is rendered, the toplevel is drawn first followed by all sub- (and subsub-) elements. * The drawing usually does not set a clipping rectangle for the respective (sub)item but requires these to do their drawing in their reserved space. To perform proper clipping would have required to revise many places in the code in order to set a clipping rectangle on the many graphics contexts involved at many places where specific draw operations are carried out. * The olden tk widgets each have their own drawing strategy and do that in an order given by explicit code. * The olden tk widgets therefore would have circumvented the ttk problem by drawing the focus ring and the enclosing relief last in their drawing implementation. * IMO we see a design problem of ttk here: although much energy was spent to allow modelling of parent/child relationships of parts of a layout the aspect (or even requirement) of clipping was neglected. nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 16:56:57: I have found the function needed for retrieving the current theme: Ttk_GetCurrentTheme() (see ttkTheme.c). nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 16:44:28: I think, "[NSApp macOSVersion] > 100800" means "OS version > 10.8", which is nowadays fulfilled on practically all Macs (e.g., my Mac runs macOS 14.5). nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 16:23:36: To get the answer to your questions regarding the aqua theme, please take a look at the functions TreeAreaElementSize() and TreeHeaderElementDraw() in the file ttkMacOSXTheme.c. Without decrementing y and incrementing height by 4, the clipping code draws a white rectangle of height 4 above the heading area. It took me pretty long to find out that this is due to the (undocumented) details mentioned above. Looking again at ttkMacOSXTheme.c, now I see that the above is only valid for sufficiently current macOS versions (Marc could tell us how "[NSApp macOSVersion] > 100800" translates in "$tcl_platform(osVersion) > ???"). But I have no idea how to retrieve the current theme in C, or at least to check whether or not the current theme is aqua. sbron added on 2024-08-26 15:25:18: Thanks for the additions, Csaba. I have a few observations that I'm hoping you can clear up: I noticed that you added a "disp" variable. I had considered doing that myself. But then I found that "Tk_Display(tkwin)" is a preprocessor macro that maps to a pointer in a struct. I think the compiler will do all of the calculations needed for the indirections. So there shouldn't be a need to cache the value in a variable for efficiency reasons. The x/y/height juggling looks a bit like a hack. Why are y and height wrong in the aqua theme? Why is the rest of the code not affected by that? Where does the 4 come from? Is that fixed? nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 15:23:03: Sorry, "the compiler complied" -> "the compiler complained". nemethi (claiming to be Csaba Nemethi) added on 2024-08-26 13:36:24: I think there is no need for rebasing the branch to 9.0. I am attaching a slightly modified version of the file ttkTreeview.c, which I have successfully tested on Linux and macOS. In the aqua theme the field element, and thus also the ttk::treeview widget, has no border, but the user might have modified its padding, hence the clipping is needed for this theme, too. The attached code makes use of the variables x and y to distinguish this theme from the others (unfortunately, I am not aware of any public function designed to get the name of the current theme). Note the special handling of the variables y and height, which is necessary in case of the aqua theme. In addition, for Tk 8.6 (only) the code snippet #ifdef _WIN32 #include "tkWinInt.h" #endif at the beginning of the file had to be extended to become #ifdef _WIN32 #include "tkWinInt.h" #elif defined(MAC_OSX_TK) #include "tkMacOSXInt.h" #endif This is so the compiler can find the declaration of the function TkpClipDrawableToRect(). Finally, on macOS the compiler complied about the already defined macro MAX. Since this macro was used at just one place in the code, I removed it (and adapted that code place accordingly). sbron added on 2024-08-26 10:25:26: I have made the changes on the "bug-d82fa2953a" branch (Tk 8.6). I am not sure how to rebase the branch to 9.0. Fossil doesn't seem to support rebasing by design. I can make a new branch off trunk with the same functionality. jan.nijtmans added on 2024-08-26 09:25:05: Thank you all for doing this work! I suggest to rebase the "bug-d82fa2953a" branch to 9.0, and try to make it work there. I'm willing to test it on MacOS. After that, it can be backported to 8.6 (and 8.7) with whatever additional changes are required. Hope this helps, Jan Nijtmans sbron added on 2024-08-26 09:10:41: I had been wondering if this method would work on all platforms. Apparently MacOS need special attention. Windows is fine? The required change seems quite straight forward, but I have no means to test it. And I don't trust myself to write flawless code without testing. In the past I think I once managed to make 3 typos in a 2-character code change. I'll be happy to take a stab at adding the TkpClipDrawableToRect() call, but I will have to rely on someone else to make sure it works. nemethi (claiming to be Csaba Nemethi) added on 2024-08-25 16:45:48: Hi Marc, you are right again! Yes, we need to use TkpClipDrawableToRect() (Tk_ClipDrawableToRect() in Tk 9) in order to protect the widget's border if any. marc_culler (claiming to be Marc Culler) added on 2024-08-25 16:16:57: Hi Csaba, don't we have to worry about the other themes, which are supposed to work on macOS as well? nemethi (claiming to be Csaba Nemethi) added on 2024-08-25 15:32:13: Marc, you are right, the clipping code must be deactivated on macOS. In general one would use TkpClipDrawableToRect() (Tk_ClipDrawableToRect() in Tk 9) instead, but in this special case there is no need for it because on macOS the field element (and thus also the ttk::treeviw widget) has no border. marc_culler (claiming to be Marc Culler) added on 2024-08-24 21:41:42: Csaba, did you test the code on macOS as well? I am concerned about the calls to XCopyArea and the use of a pixmap as a temporary drawing surface. Possibly that would work in Tk 9 with the new cgimage drawing, I am not sure. But in core-8-6-branch I believe that calls to XCopyArea are generally inside blocks of condition code controlled by #ifdef NO_DOUBLE_BUFFERING. The macOS code is invoked when NO_DOUBLE_BUFFERING is defined. That code generally draws directly to the screen, not to a temporary pixmap. I am not seeing that compiler constant in the new code. Some of the new code is for clipping the drawing. A simple alternative for clipping the drawing on macOS would be to add a clipping rectangle to the macOS graphics context before doing the drawing. When the graphics context is popped at the end of the drawing the previous clipping region would be restored. nemethi (claiming to be Csaba Nemethi) added on 2024-08-24 17:40:42: I have successfully tested the latest commit (of today). Everything appears to work as expected. The clipping not only protects the border but is also free from the artifacts experienced with earlier versions. I think the code is now in a quite good shape, ready for merge. chw added on 2024-08-23 11:52:08: Why not try out my ersatz clipping strategy then? sbron added on 2024-08-23 09:51:15: Checking with Tcl/Tk 8.6.13, I see that the ttk::treeview widget has always trampled over its border. Just only on the left and right sides, because it cowardly shied away from drawing the bottom row if it didn't completely fit. sbron added on 2024-08-22 12:46:53: You need better eyes than mine to notice that. But I **can** see that it draws over the border when I make the border much wider using:
```tcl
ttk::style configure Treeview -borderwidth 8
```
chw added on 2024-08-22 10:04:58: Although bug-d82fa2953a fixes the line spacing it still draws into the bottom of the focus ring. Can be seen best with the "classic" theme in unfocused state. I'm afraid it requires to clip properly or perform some other trickery as I've tried in my approach. sbron added on 2024-08-22 09:46:45: Christian, Can you try the resurrected bug-d82fa2953a branch? I believe that fixes the issue. chw added on 2024-08-22 09:01:40: Schelte, the easiest way to see it is in the directory tree widget demo. Just resize the toplevel and observe strange effects regarding the line spacing and the redraw of the indicator triangle. sbron added on 2024-08-22 08:43:27: I think I see now. When interactively resizing the height of the window, the bottom item moves. Strange. The focus ring is not affected, though. Csaba, in case you are looking into it: I think I already have a fix for the horizontal scrolling issue. sbron added on 2024-08-22 08:14:51: I'm sorry, I don't understand the issue. The height of the bounding box of the last row is not shrunk. It is the same as any other row, even if it is only partly visible. Can you explain more, maybe with a small script that shows the problem? There are actually 2 focus rings: One around the treeview widget and one around the current item. Which of these two are you referring to? The only artifact I did notice while scrutinizing the widget again is that the left side of the focus ring is broken when the widget is scrolled horizontally. But I don't think that is what you are talking about, are you? It will have to be addressed though. I therefor reopened the ticket. chw added on 2024-08-22 04:18:38: Schelte, at least in 8.6 (possibly in trunk, too, but I didn't test) the focus ring gets damaged at the bottom when the last row is partial and the row bounding box is left as is in order to get its layout proper. I think the only possible fix is to introduce some kind of clipping like so: https://www.androwish.org/home/fdiff?v1=6aaa17eeedc37461&v2=1d923cdcd5a1a926 If the height of the bounding box of the last row is shrunk as in your latest change, the spacing between the last row and the row before is not equal to the overall row spacing, which is not visually pleasing. sbron added on 2024-08-16 18:37:36: Fixes merged to core-8-6-branch, core-8-branch, and trunk. nemethi (claiming to be Csaba Nemethi) added on 2024-08-16 18:13:34: As a positive side effect, the fix for this ticket has eliminated the bug reported in [56f6a06179], too. With Tk built from the latest version of core-8-branch or trunk, that issue is no longer reproducible. nemethi (claiming to be Csaba Nemethi) added on 2024-08-16 14:04:25: No objections from my side. sbron added on 2024-08-16 13:48:47: The 'bbox' command could already return an area that exceeds the width of the widget. The fact that it can now also exceed the height of the widget doesn't really make things worse. So just documenting this fact in the manual page is the easiest way to deal with it (done). I don't see any new test suite failures. I think the branch is ready to be merged. Any objections? sbron added on 2024-08-16 10:48:34: That's fixed now too. sbron added on 2024-08-16 10:41:51: The problem seems to be that TreeWidth(tv) initially returns the sum of the requested widths of all columns. After resizing back and forth, the column sizes have been adjusted. But that is only done if they are stretchable. With [.wut column #0 -stretch 0], the focus ring gets broken again when the widget is resized to something smaller than its requested width. nemethi (claiming to be Csaba Nemethi) added on 2024-08-16 10:21:34: If you insert, say, only 20 items rather than 50 and make the window interactively higher until all the items are visible, then you will see that below the items the right side of the focus ring is visible. Apparently, the items overlap the right edge of the border. A similar problem like issue #3, but horizontally rather than vertically. sbron added on 2024-08-16 09:22:41: So a similar remark may have to be added in the treeview manual page. And hopefully the last remark: When I run the attached demo script, the width of the treeview widget is 135, while the requested width is 204. The right side of the focus ring is missing. When I use the mouse to make the window bigger, the focus ring is drawn correctly as soon as the width reaches the requested width. After that, I can make the window smaller again and the focus ring remains fully visible. But apparently something is different when the widget is created with a width that is smaller than it would like. Surprisingly it's not a problem if the height is too small. nemethi (claiming to be Csaba Nemethi) added on 2024-08-16 08:55:48: I agree. In the case of the listbox it is explicitly mentioned in the manual that "if the element is partially visible, the result gives the full area of the element, including any parts that are not visible". sbron added on 2024-08-16 08:22:39: I think the reported issues have been fixed in the bug-d82fa2953a branch. One other minor point is that the 'bbox' command reports the full item height for the item that is only partially visible at the bottom. It seems more logical if it would only report the visible height. But the old-fashioned listbox does exactly the same and nobody seems to mind that. So I wonder if it is worth the effort to add code to calculate the proper height. sbron added on 2024-08-15 20:17:55: Your assistance is much appreciated, especially with respect to issue #3. Regarding issue #2, see the attached demo script. If you run this with Tcl/Tk 8.6.13 and scroll down a page at a time, the treeview widget will show items 1..10 and a tiny bit of item 11 on the first page, items 12..21 on the second page, 23..32 on the third page, etc. So the user doesn't really get to see item 11, 22, 33. If you change wut to "listbox" or "text", you can see that they also show items 1..10 on the first page, but then they show items 9..18 on the second page, then 17..26, etc. So there is some overlap. With the treeview widget there is "underlap". This is just for your understanding. I have already committed a fix to the bug-d82fa2953a branch. nemethi (claiming to be Csaba Nemethi) added on 2024-08-15 19:57:31: OK, I have managed to close the fork. I am awfully sorry for the mistake! sbron added on 2024-08-15 19:52:51: I mentioned in the ticket that I would look into it and I assigned the ticket to myself. I don't know what more I could have done to prevent duplicated effort. I also don't have the required knowledge to eliminate a fork. nemethi (claiming to be Csaba Nemethi) added on 2024-08-15 19:24:18: I have just made a mad mistake, sorry! I didn't see your changes and checked in my own version for fixing issue #1, thus inadvertently creating a fork. Can somebody please help me to eliminate the fork? I am too stupid for that task. nemethi (claiming to be Csaba Nemethi) added on 2024-08-15 16:27:32: Regarding issue #1 you are right: The see command should make the item fully vertically visible in the window. nemethi (claiming to be Csaba Nemethi) added on 2024-08-15 16:08:16: I'm not sure I understand correctly what you mean by issue #2. The listbox (and also the tablelist widget) behave the same. sbron added on 2024-08-15 15:16:53: Confirmed. Issue #3 is solved by that change. nemethi (claiming to be Csaba Nemethi) added on 2024-08-15 15:00:50: Please check whether the commit [87ea6403] solves the reported issues. At least the ones related to the bottom should no longer be present in Tk built from the branch bug-d82fa2953a. | |||