Removal of macOS-specific code by Gregor Cramer

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Removal of macOS-specific code by Gregor Cramer

Kevin Walzer-3

Hi Gregor,

Contacting you here because I'm not sure how to add comments to a commit.

I am very concerned about the changes you made in http://core.tcl.tk/tk/info/a232ec01e6b7266f, touching Mac-specific code in your revised-text branch. That code was the result of many months of work by myself and Marc Culler to improve the performance of both the text widget and drawing performance of Tk on MacOS in general, and you have offered no basis (as far as I can tell) to remove this code except to state that these bits should not be in the generic implementation:  "#ifdef MAC_OSX_TK" code replaced, the generic implementation should not contain platform specific code (only debugging code is an exception)."

I understand the desire not to litter the text widget itself with these #ifdefs. When working with Marc on these changes, I did not commit every suggestion he made, only those that seemed walled off from the generic bits and made a noticable difference in the text widget on MacOS. I would also give deference to the text widget maintainers if these were the only changes being requested. However, you are also modifying the Mac-specific drawing module itself, for reasons that I cannot fathom.

My concern is this: removing this code willy-nilly may cause a degradation in the drawing and event processing performance of Tk on MacOS, in both the text widget and elsewhere. Have you carefully tested these changes to make sure there are no side effects? Tk has suffered from substantial issues in drawing and event loop handling on the Mac for the past several years, and the very bits you are ripping out went a long way to improving the performance. A regression for no reason other than the code organization seemed a bit messy is intolerable.

I would ordinarily be quite willing to test the changes before commenting, but I cannot currently get the revised_text branch to build and run without crashing. As such, please clarify your reasons for making these edits. If I am not persuaded enough by the rationale, as the Mac maintainer of Tk I will ask you to revert them.

Thank you,

Kevin

--
Kevin Walzer
Code by Kevin/Mobile Code by Kevin
http://www.codebykevin.com
http://www.wtmobilesoftware.com

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Tcl-Core mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/tcl-core
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Removal of macOS-specific code by Gregor Cramer

Gregor Cramer
> I am very concerned about the changes you made in
> http://core.tcl.tk/tk/info/a232ec01e6b7266f, touching Mac-specific code
> in your revised-text branch. That code was the result of many months of
> work by myself and Marc Culler to improve the performance of both the
> text widget and drawing performance of Tk on MacOS in general, and you
> have offered no basis (as far as I can tell) to remove this code except
> to state that these bits should not be in the generic implementation:
> "#ifdef MAC_OSX_TK" code replaced, the generic implementation should not
> contain platform specific code (only debugging code is an exception)."

At first I will explain the changes. The goal of these changes was to  transfer
Max-specific code in text widget implementation to the Mac interface. Only
three places in text widget are concerned:

-----------------------------------------------------------------------------------------

#ifdef MAC_OSX_TK
TkWindow *winPtr = (TkWindow *)(textPtr->tkwin);
    MacDrawable *macWin = winPtr->privatePtr;
    if (macWin && (macWin->flags & TK_DO_NOT_DRAW)) {
    ...
}
#endif

has been replaced with the use of a new (primitive) function:

if (TkpDrawingIsDisabled(textPtr->tkwin)) {
    ...
}

This is the Mac implementation:

int
TkpDrawingIsDisabled(
   Tk_Window tkwin)
{
    MacDrawable *macWin = ((TkWindow *) tkwin)->privatePtr;
    return macWin && !!(macWin->flags & TK_DO_NOT_DRAW);
}

All other platforms are using a simple macro:
#define TkpDrawingIsDisabled(tkwin) 0
-----------------------------------------------------------------------------------------

The code in text widget

#ifdef MAC_OSX_TK
    /* Don't show inactive selection in disabled widgets. */
     if (textPtr->state == TK_TEXT_STATE_DISABLED) {
        continue;
    }
#endif

has  been replaces with the following code:

 if (textPtr->state == TK_TEXT_STATE_DISABLED &&
    *DEF_TEXT_INACTIVE_SELECT_COLOR_DISABLED == '1') {
        continue;
}

Only the Mac version is defining the additional constant
DEF_TEXT_INACTIVE_SELECT_COLOR_DISABLED  (tk*Default.h) with value "1", all
other platforms are using value "0".

-----------------------------------------------------------------------------------------

In function TkScrollWindow (tkMacOSXDraw.c) I've factorized the Exposure
generation:

#if 0 /* Don't queue Expose event, otherwise it is not compatible to UNIX and
Windows version. */
    /* Queue up Expose events for the damage region. */
    int oldMode = Tcl_SetServiceMode(TCL_SERVICE_NONE);
    [view generateExposeEvents:dmgRgn childrenOnly:1];
    Tcl_SetServiceMode(oldMode);
#endif

The generation of an Exposure event must be a mistake, because the UNIX and
Windows implementation do not trigger Exposure events when scrolling, I think
that the Mac version should be conform. Currently only the text widget is
using this function, so no other widget can be affected. Before we had the
following code in text widget:

if (TkScrollWindow(..., damageRgn)) {
#ifdef MAC_OSX_TK
    /* the processing of the Expose event is damaging the region on Mac */
#else
    TextInvalidateRegion(textPtr, damageRgn);
#endif

This has been changed to a solid code:

if (TkScrollWindow(..., damageRgn)) {
    TextInvalidateRegion(textPtr, damageRgn);
}

because now all platform specific implementations of TkScrollWindow() are
working equally. Please note that instead of a removal  I've used
factorization, the code for event generation is still contained.

-----------------------------------------------------------------------------------------
 
> Have you carefully tested these changes to make sure there are no side
effects?

Testing was easy, because only the text widget is affected.

I understand your concerns as the maintainer of the Tk code, and now I know
that asking you before doing such changes is the better way. Of course, for
the text widget to revert (some of) the changes wouldn't be a problem, but
probably you may agree with the simple changes I have done. If you have
further questions about the changes, please don't hesitate to ask.

Cheers,
Gregor

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Tcl-Core mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/tcl-core
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Removal of macOS-specific code by Gregor Cramer

Kevin Walzer-3
Gregor,

Thank you for clarifying. So these changes are largely confined to the text widget.  I will still test when I get a chance, but I have a better understanding of your goals now; you have addressed my concern.

Thanks,
Kevin

On May 25, 2017, at 9:50 AM, Gregor Cramer <[hidden email]> wrote:

>> I am very concerned about the changes you made in
>> http://core.tcl.tk/tk/info/a232ec01e6b7266f, touching Mac-specific code
>> in your revised-text branch. That code was the result of many months of
>> work by myself and Marc Culler to improve the performance of both the
>> text widget and drawing performance of Tk on MacOS in general, and you
>> have offered no basis (as far as I can tell) to remove this code except
>> to state that these bits should not be in the generic implementation:
>> "#ifdef MAC_OSX_TK" code replaced, the generic implementation should not
>> contain platform specific code (only debugging code is an exception)."
>
> At first I will explain the changes. The goal of these changes was to  transfer
> Max-specific code in text widget implementation to the Mac interface. Only
> three places in text widget are concerned:
>
> -----------------------------------------------------------------------------------------
>
> #ifdef MAC_OSX_TK
> TkWindow *winPtr = (TkWindow *)(textPtr->tkwin);
>    MacDrawable *macWin = winPtr->privatePtr;
>    if (macWin && (macWin->flags & TK_DO_NOT_DRAW)) {
>    ...
> }
> #endif
>
> has been replaced with the use of a new (primitive) function:
>
> if (TkpDrawingIsDisabled(textPtr->tkwin)) {
>    ...
> }
>
> This is the Mac implementation:
>
> int
> TkpDrawingIsDisabled(
>   Tk_Window tkwin)
> {
>    MacDrawable *macWin = ((TkWindow *) tkwin)->privatePtr;
>    return macWin && !!(macWin->flags & TK_DO_NOT_DRAW);
> }
>
> All other platforms are using a simple macro:
> #define TkpDrawingIsDisabled(tkwin) 0
> -----------------------------------------------------------------------------------------
>
> The code in text widget
>
> #ifdef MAC_OSX_TK
>    /* Don't show inactive selection in disabled widgets. */
>     if (textPtr->state == TK_TEXT_STATE_DISABLED) {
>        continue;
>    }
> #endif
>
> has  been replaces with the following code:
>
> if (textPtr->state == TK_TEXT_STATE_DISABLED &&
>    *DEF_TEXT_INACTIVE_SELECT_COLOR_DISABLED == '1') {
>        continue;
> }
>
> Only the Mac version is defining the additional constant
> DEF_TEXT_INACTIVE_SELECT_COLOR_DISABLED  (tk*Default.h) with value "1", all
> other platforms are using value "0".
>
> -----------------------------------------------------------------------------------------
>
> In function TkScrollWindow (tkMacOSXDraw.c) I've factorized the Exposure
> generation:
>
> #if 0 /* Don't queue Expose event, otherwise it is not compatible to UNIX and
> Windows version. */
>    /* Queue up Expose events for the damage region. */
>    int oldMode = Tcl_SetServiceMode(TCL_SERVICE_NONE);
>    [view generateExposeEvents:dmgRgn childrenOnly:1];
>    Tcl_SetServiceMode(oldMode);
> #endif
>
> The generation of an Exposure event must be a mistake, because the UNIX and
> Windows implementation do not trigger Exposure events when scrolling, I think
> that the Mac version should be conform. Currently only the text widget is
> using this function, so no other widget can be affected. Before we had the
> following code in text widget:
>
> if (TkScrollWindow(..., damageRgn)) {
> #ifdef MAC_OSX_TK
>    /* the processing of the Expose event is damaging the region on Mac */
> #else
>    TextInvalidateRegion(textPtr, damageRgn);
> #endif
>
> This has been changed to a solid code:
>
> if (TkScrollWindow(..., damageRgn)) {
>    TextInvalidateRegion(textPtr, damageRgn);
> }
>
> because now all platform specific implementations of TkScrollWindow() are
> working equally. Please note that instead of a removal  I've used
> factorization, the code for event generation is still contained.
>
> -----------------------------------------------------------------------------------------
>
>> Have you carefully tested these changes to make sure there are no side
> effects?
>
> Testing was easy, because only the text widget is affected.
>
> I understand your concerns as the maintainer of the Tk code, and now I know
> that asking you before doing such changes is the better way. Of course, for
> the text widget to revert (some of) the changes wouldn't be a problem, but
> probably you may agree with the simple changes I have done. If you have
> further questions about the changes, please don't hesitate to ask.
>
> Cheers,
> Gregor


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Tcl-Core mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/tcl-core
Loading...