Tk Source Code

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83

Le 13/01/2019 � 12:21, Francois Vogel a �crit : > Hi Marc, > > Thanks for this work, once more! A few thoughts on this. > > > 1 - Code review > > Preliminary remark: > > I wanted to look at the changes in the bugfix branch at once (concatenating all commits there instead of reviewing each of them one after the other, the nextone having changed the changes of a previous one and so on...). So in the timeline of the bugfix branch: > > https://core.tcl-lang.org/tk/timeline?r=bug-70e531918e > > I clicked the round dot representing the commit (at the very bottom) from which the branch originates, and then I clicked the round dot of the latest commit. This gives me a diff between these two commits and is very handy. However, since you merged core-8-6-branch in the bugfix branch I ended up in getting huge changes that are unrelated to what you want me to review. What was the reason for merging core-8-6-branch in the bugfix branch? Did core-8-6-branch contain any fix that is useful in our bugfix branch perhaps? > > So I reviewed the changes in smaller steps, and here are some comments. > > - Too bad to have to add #ifdef TK_MAC_OSX in generic code > > - Could we get rid of the hardcoded tweaking values, such as those in tk::PostMenubuttonMenu, or those identified by /* tweak */ in the mac code? How robust are those...? > > - Are the changes of the defaults in tkMacOSXDefault.h for instance backwards compatible? Will those changes break user code in the wild? > > - I didn't understand the test on txtHeight != 2 (again a hardcoded special value) in tkMacOSXMenubutton.c:280 (this test was against zero previously) > > > Test suite results in the bugfix branch: > > - 100% pass on Windows > > - menubut.test tests now all pass on macOS (Mojave) and no new test is failing (this is modulo the known uncertainties about tests instabilities, of course) > > - no change in the test suite results on Linux Debian 8 (again, modulo instabilities), menubut.test passes 100% > > > Demo comparison between 8.5.16 I have installed here, and the bugfix branch, using the "Menus and Toolbars/Menu-buttons" demo: > > - I see the fixes you described in the ticket [70e531918e], for both Windows and Linux. Everything looks good in that demo on those platforms. > > - on macOS (Mojave), the rendering is different from the other two platforms: > > a - "Left" and "Right" menues open slightly above the top border of the button (this is not the case on the other platforms). Perhaps not a problem: The menu item is correctly aligned with the menubutton, but the menu border is higher. > > b - the color picker is not a palette (a more or less square thing) like on the other platforms. This is because the -columnbreak option is not supported on the mac. Moreover the tearoff entry is not available, again because this feature is not available on the mac. > > > > 2 and 3 - Extension to the menu post command and TIP > > I understand the good reason why you did this. However, as any extension of the language, this would require a TIP. I don't expect this to be controversial at all, it should be very easy and quick tough. I will happily sponsor it, yes. But we definitely should write a small TIPlet. We require others to do it in such cases, we cannot save the effort here I think/ > Normally, such an extension TIP would target trunk only (that is 8.7), because the 8.6 line is not supposed to receive new features, only bugfixes. In this specific case however, for the reasons you stated, I think we can push this into 8.6.x. Again, that explanation needs to go in the TIP. > > There is no new test covering the added feature, could we imagine such a test? At least if we cannot check the rendering (really...?) we should test the new syntax a bit better than the change in the error message done in menu-3.47 to make this test pass. > > The new feature is available at the Tcl level. Should it be available also at the Tk C API level, and if so, where? Did you think about this already perhaps? > > > All that said, I'm wondering how much of these comments pertain to the ticket. I will perhaps copy/paste some of the above text in the ticket. > > Thanks, > F. > > > Le 11/01/2019 � 22:59, Marc Culler a �crit : >> Hi Kevin and Fran�ois, >> >> My little project of fixing the test errors in menubutton.test became a much bigger project after I actually looked at how they were working in the demo and found that they were pretty broken on all platforms. I believe that I have now fixed menubuttons on all platforms. (See the ticket 70e531918e .) >> >> So I have three questions for the two of you: >> >> 1. Would you please review the code? >> >> 2. I ended up extending Tk ever so slightly, by adding one optional parameter to the "menu post" command. You can specify an index, and then the menu will be posted so that the top left corner of that entry is at the point, instead of the top left corner of the whole menu. On the Mac his allows me to use a built in method of NSMenu , which should be much more robust than trying to compute the geometry of Apple's proprietary menu. (I can and do compute the geometry correctly today, but who knows what will happen with the next Apple OS release?) >> >> Anyway, the question is: does this require a TIP? >> >> 3. If the answer to #2 is yes, will you sponsor a TIP? >> >> Thanks. >> >> - Marc