From: Jim D. <ji...@di...> - 2015-06-16 01:56:13
|
And here is the wingcc patch series. I tested with cygwin and msvc and did not have the problem with multiple keypresses. The wingcc driver does seem to have a problem when the window is made small. It cuts off part of the plot. > On Jun 15, 2015, at 12:24 PM, Jim Dishaw <ji...@di...> wrote: > >> >> On Jun 14, 2015, at 1:40 PM, Jim Dishaw <ji...@di...> wrote: >> >> >>> On Jun 14, 2015, at 5:19 AM, Alan W. Irwin <ir...@be...> wrote: >>> >>> On 2015-06-13 22:29-0400 Jim Dishaw wrote: >>> >>>> >>>>> On Jun 13, 2015, at 2:11 PM, Jim Dishaw <ji...@di...> wrote: >>>>> >>>>> >>>>> On Jun 13, 2015, at 1:13 PM, Phil Rosenberg <p.d...@gm... <mailto:p.d...@gm...>> wrote: >>>>> >>>>>> Hmmm >>>>>> Then hmmmm some more >>>>>> Followed by an ummm or two. >>>>>> >>>>>> As you say Jim, clearly things are more complicated than I realised. I'm not sure what the requirements really are here now. Now you have brought this up I can imagine the issues associated with entering the message loop after an eop. >>>>>> >>>>>> In terms of dealing with it, I'm not at my computer right now, but I think probably a bop_forced function which forces a bop irrespective of where we are in the page cycle would work. But I'm not sure if that might have some potential subtle effects elsewhere so it is a bit of a hack really. Perhaps instead it would be better to assess what is being done in a bop and ensure those actions end up in the buffer so a bop event is not required. >>>>>> >>>>> >>>>> I think that is a good approach for reasons beyond this issue, but a BOP and EOP of some type is still required to support the drivers adequately (e.g double buffering, printing from the windows API). >>>>> >>>>> The solution I'm going to test is one where the driver does not enter into a wait for input state on a redraw event from the callback. >>>> >>>> I came up with two solutions, one of which I tested. >>>> >>>> Solution 1 (Tested) >>>> >>>> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so now an EOP is written to the buffer. This will cause an EOP to be generated when the plot buffer is replayed. I like the symmetry of having an EOP in the buffer to match the BOP. >>>> >>>> Drivers will be responsible for determining if a “wait for user input” should be performed during an EOP. For the wingdi driver I track the state of a “page_state” that changes outside of the BOP/EOP calls. >>>> >>>> This solution feels a bit kludgy, but it works. >>>> >>>> Solution 2 >>>> >>>> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so now an EOP is written to the buffer. This will cause an EOP to be generated when the plot buffer is replayed. >>>> >>>> Add a new driver function to the dispatch table (e.g. pl_wait) that calls the "wait for user input between pages" function. >>>> >>>> Remove from all drivers the “wait for input” from the EOP handler. >>>> >>>> The PLplot core library will call the wait for input between pages function (if not null). >>>> >>>> This solution strikes me as a more elegant solution because the logic for deciding about the wait for input is one spot. >>>> >>> >>>> Regardless of the solution, some changes to the drivers will be >>> required. Solution 2 will touch all the drivers (at a minimum to add >>> a NULL to the dispatch table). Solution 1 adds code to the drivers >>> while solution 2 removes some code from the drivers. >>> >>> Hi Jim: >>> >>> Thanks for your effort working through possible solutions to the >>> problem. >>> >>> @Phil and Andrew: >>> >>> Assuming you guys agree with Jim's analysis, do you prefer solution 1 >>> or 2? Elegant and "removes some code from the drivers" (i.e., >>> solution 2) seems like the better choice to me from an overview >>> perspective, but I would be happy to go along with whatever you all >>> decide since I do not have the driver expertise you guys have. >>> >>> Assuming whatever solution is decided upon here can be implemented >>> in a reasonably timely way, then I think we should be able to fit >>> it into the forthcoming bug-fix release since this is obviously a >>> bug fix, albeit a rather intrusive one. >>> >>> With regard to timing for that release there is still quite a bit I >>> plan to do to work through the bugs in the build system revealed by >>> previous tests on Linux and Arjen's current tests on the Cygwin >>> platform. And the new tarball report that is automatically collected >>> by scripts/comprehensive_test.sh should make reporting of tests and >>> figuring out build-system issues from those reports _much_ easier. >>> Which likely means this release will be tested on a lot more platforms >>> than the last one and a significant amount of time will be required to >>> deal with any build system bugs that are turned up by such tests. So >>> my current guestimate is the forthcoming release is likely more than a >>> month away (although hopefully not longer than two months away). >>> > > Attached is a patch series that implements the second solution. I tested the changes to xwin driver; however, I was not able to test tkwin, qt, or cairo. I will provide an update to wingcc separately. I didn’t touch the wxwidgets driver. > > I didn’t have to touch the drivers that do not need the wait function pointer (e.g. postscript). The way the driver dispatch table is initialized, I was able to set all the function pointers to NULL in the plcore.c file. The drivers set each member individually, thus the wait function pointer is left NULL. > > The xwin driver occasionally misses some resizes, but I think that is some quirky behavior that existed prior to 5.10. > > If someone could test the tkwin, qt, and cairo (specifically xcairo) that would be greatly appreciated. > > > <0001-Fix-to-the-multiple-keypress-bug-on-page-advance.patch> > > > ------------------------------------------------------------------------------ > _______________________________________________ > Plplot-devel mailing list > Plp...@li... <mailto:Plp...@li...> > https://lists.sourceforge.net/lists/listinfo/plplot-devel <https://lists.sourceforge.net/lists/listinfo/plplot-devel> |
From: Phil R. <p.d...@gm...> - 2015-06-16 08:58:26
|
So how should we go about applying all this? Will we break some drivers when it is applied? If so do we need to have all drivers prepared so that we can apply all the fixes together? Phil On 16 June 2015 at 02:55, Jim Dishaw <ji...@di...> wrote: > And here is the wingcc patch series. I tested with cygwin and msvc and did > not have the problem with multiple keypresses. The wingcc driver does seem > to have a problem when the window is made small. It cuts off part of the > plot. > > > > > On Jun 15, 2015, at 12:24 PM, Jim Dishaw <ji...@di...> wrote: > > > On Jun 14, 2015, at 1:40 PM, Jim Dishaw <ji...@di...> wrote: > > > On Jun 14, 2015, at 5:19 AM, Alan W. Irwin <ir...@be...> > wrote: > > On 2015-06-13 22:29-0400 Jim Dishaw wrote: > > > On Jun 13, 2015, at 2:11 PM, Jim Dishaw <ji...@di...> wrote: > > > On Jun 13, 2015, at 1:13 PM, Phil Rosenberg <p.d...@gm... > <mailto:p.d...@gm...>> wrote: > > Hmmm > Then hmmmm some more > Followed by an ummm or two. > > As you say Jim, clearly things are more complicated than I realised. I'm not > sure what the requirements really are here now. Now you have brought this up > I can imagine the issues associated with entering the message loop after an > eop. > > In terms of dealing with it, I'm not at my computer right now, but I think > probably a bop_forced function which forces a bop irrespective of where we > are in the page cycle would work. But I'm not sure if that might have some > potential subtle effects elsewhere so it is a bit of a hack really. Perhaps > instead it would be better to assess what is being done in a bop and ensure > those actions end up in the buffer so a bop event is not required. > > > I think that is a good approach for reasons beyond this issue, but a BOP and > EOP of some type is still required to support the drivers adequately (e.g > double buffering, printing from the windows API). > > The solution I'm going to test is one where the driver does not enter into a > wait for input state on a redraw event from the callback. > > > I came up with two solutions, one of which I tested. > > Solution 1 (Tested) > > Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so > now an EOP is written to the buffer. This will cause an EOP to be generated > when the plot buffer is replayed. I like the symmetry of having an EOP in > the buffer to match the BOP. > > Drivers will be responsible for determining if a “wait for user input” > should be performed during an EOP. For the wingdi driver I track the state > of a “page_state” that changes outside of the BOP/EOP calls. > > This solution feels a bit kludgy, but it works. > > Solution 2 > > Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so > now an EOP is written to the buffer. This will cause an EOP to be generated > when the plot buffer is replayed. > > Add a new driver function to the dispatch table (e.g. pl_wait) that calls > the "wait for user input between pages" function. > > Remove from all drivers the “wait for input” from the EOP handler. > > The PLplot core library will call the wait for input between pages function > (if not null). > > This solution strikes me as a more elegant solution because the logic for > deciding about the wait for input is one spot. > > > Regardless of the solution, some changes to the drivers will be > > required. Solution 2 will touch all the drivers (at a minimum to add > a NULL to the dispatch table). Solution 1 adds code to the drivers > while solution 2 removes some code from the drivers. > > Hi Jim: > > Thanks for your effort working through possible solutions to the > problem. > > @Phil and Andrew: > > Assuming you guys agree with Jim's analysis, do you prefer solution 1 > or 2? Elegant and "removes some code from the drivers" (i.e., > solution 2) seems like the better choice to me from an overview > perspective, but I would be happy to go along with whatever you all > decide since I do not have the driver expertise you guys have. > > Assuming whatever solution is decided upon here can be implemented > in a reasonably timely way, then I think we should be able to fit > it into the forthcoming bug-fix release since this is obviously a > bug fix, albeit a rather intrusive one. > > With regard to timing for that release there is still quite a bit I > plan to do to work through the bugs in the build system revealed by > previous tests on Linux and Arjen's current tests on the Cygwin > platform. And the new tarball report that is automatically collected > by scripts/comprehensive_test.sh should make reporting of tests and > figuring out build-system issues from those reports _much_ easier. > Which likely means this release will be tested on a lot more platforms > than the last one and a significant amount of time will be required to > deal with any build system bugs that are turned up by such tests. So > my current guestimate is the forthcoming release is likely more than a > month away (although hopefully not longer than two months away). > > > Attached is a patch series that implements the second solution. I tested > the changes to xwin driver; however, I was not able to test tkwin, qt, or > cairo. I will provide an update to wingcc separately. I didn’t touch the > wxwidgets driver. > > I didn’t have to touch the drivers that do not need the wait function > pointer (e.g. postscript). The way the driver dispatch table is > initialized, I was able to set all the function pointers to NULL in the > plcore.c file. The drivers set each member individually, thus the wait > function pointer is left NULL. > > The xwin driver occasionally misses some resizes, but I think that is some > quirky behavior that existed prior to 5.10. > > If someone could test the tkwin, qt, and cairo (specifically xcairo) that > would be greatly appreciated. > > > <0001-Fix-to-the-multiple-keypress-bug-on-page-advance.patch> > > > ------------------------------------------------------------------------------ > _______________________________________________ > Plplot-devel mailing list > Plp...@li... > https://lists.sourceforge.net/lists/listinfo/plplot-devel > > > > ------------------------------------------------------------------------------ > > _______________________________________________ > Plplot-devel mailing list > Plp...@li... > https://lists.sourceforge.net/lists/listinfo/plplot-devel > |
From: Jim D. <ji...@di...> - 2015-06-16 15:11:41
|
> On Jun 16, 2015, at 4:58 AM, Phil Rosenberg <p.d...@gm...> wrote: > > So how should we go about applying all this? Will we break some > drivers when it is applied? If so do we need to have all drivers > prepared so that we can apply all the fixes together? > All function pointer are set to NULL during the dispatch table initialization, thus the wait pointer remains NULL if a device does not set it. The plP_wait() function checks nopause and for a non-NULL wait function pointer before calling the wait function pointer. Thus no action is taken if a driver does not support the wait function. For non-interactive drivers, no further action is required because they don’t wait for input. For interactive drivers, the key factor is if the driver uses plRemakePlot(). If it does, it will need to provide a wait function otherwise multiple keypresses will be required after a resize. If it does not use plRemakePlot(), the change is not critical because having a wait for input in the driver’s EOP handler will not break anything. The driver should be updated to keep the internal API consistent. For example, the tek driver is in an unsupported state and there is no need to update it. The Qt driver, on the other hand, should be updated to stay consistent with the internal API even though it does not use plRemakePlot(). If during testing we discover Qt behaves badly with this change, we can leave the changes out until a later release. > Phil > > On 16 June 2015 at 02:55, Jim Dishaw <ji...@di...> wrote: >> And here is the wingcc patch series. I tested with cygwin and msvc and did >> not have the problem with multiple keypresses. The wingcc driver does seem >> to have a problem when the window is made small. It cuts off part of the >> plot. >> >> >> >> >> On Jun 15, 2015, at 12:24 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 14, 2015, at 1:40 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 14, 2015, at 5:19 AM, Alan W. Irwin <ir...@be...> >> wrote: >> >> On 2015-06-13 22:29-0400 Jim Dishaw wrote: >> >> >> On Jun 13, 2015, at 2:11 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 13, 2015, at 1:13 PM, Phil Rosenberg <p.d...@gm... >> <mailto:p.d...@gm...>> wrote: >> >> Hmmm >> Then hmmmm some more >> Followed by an ummm or two. >> >> As you say Jim, clearly things are more complicated than I realised. I'm not >> sure what the requirements really are here now. Now you have brought this up >> I can imagine the issues associated with entering the message loop after an >> eop. >> >> In terms of dealing with it, I'm not at my computer right now, but I think >> probably a bop_forced function which forces a bop irrespective of where we >> are in the page cycle would work. But I'm not sure if that might have some >> potential subtle effects elsewhere so it is a bit of a hack really. Perhaps >> instead it would be better to assess what is being done in a bop and ensure >> those actions end up in the buffer so a bop event is not required. >> >> >> I think that is a good approach for reasons beyond this issue, but a BOP and >> EOP of some type is still required to support the drivers adequately (e.g >> double buffering, printing from the windows API). >> >> The solution I'm going to test is one where the driver does not enter into a >> wait for input state on a redraw event from the callback. >> >> >> I came up with two solutions, one of which I tested. >> >> Solution 1 (Tested) >> >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so >> now an EOP is written to the buffer. This will cause an EOP to be generated >> when the plot buffer is replayed. I like the symmetry of having an EOP in >> the buffer to match the BOP. >> >> Drivers will be responsible for determining if a “wait for user input” >> should be performed during an EOP. For the wingdi driver I track the state >> of a “page_state” that changes outside of the BOP/EOP calls. >> >> This solution feels a bit kludgy, but it works. >> >> Solution 2 >> >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so >> now an EOP is written to the buffer. This will cause an EOP to be generated >> when the plot buffer is replayed. >> >> Add a new driver function to the dispatch table (e.g. pl_wait) that calls >> the "wait for user input between pages" function. >> >> Remove from all drivers the “wait for input” from the EOP handler. >> >> The PLplot core library will call the wait for input between pages function >> (if not null). >> >> This solution strikes me as a more elegant solution because the logic for >> deciding about the wait for input is one spot. >> >> >> Regardless of the solution, some changes to the drivers will be >> >> required. Solution 2 will touch all the drivers (at a minimum to add >> a NULL to the dispatch table). Solution 1 adds code to the drivers >> while solution 2 removes some code from the drivers. >> >> Hi Jim: >> >> Thanks for your effort working through possible solutions to the >> problem. >> >> @Phil and Andrew: >> >> Assuming you guys agree with Jim's analysis, do you prefer solution 1 >> or 2? Elegant and "removes some code from the drivers" (i.e., >> solution 2) seems like the better choice to me from an overview >> perspective, but I would be happy to go along with whatever you all >> decide since I do not have the driver expertise you guys have. >> >> Assuming whatever solution is decided upon here can be implemented >> in a reasonably timely way, then I think we should be able to fit >> it into the forthcoming bug-fix release since this is obviously a >> bug fix, albeit a rather intrusive one. >> >> With regard to timing for that release there is still quite a bit I >> plan to do to work through the bugs in the build system revealed by >> previous tests on Linux and Arjen's current tests on the Cygwin >> platform. And the new tarball report that is automatically collected >> by scripts/comprehensive_test.sh should make reporting of tests and >> figuring out build-system issues from those reports _much_ easier. >> Which likely means this release will be tested on a lot more platforms >> than the last one and a significant amount of time will be required to >> deal with any build system bugs that are turned up by such tests. So >> my current guestimate is the forthcoming release is likely more than a >> month away (although hopefully not longer than two months away). >> >> >> Attached is a patch series that implements the second solution. I tested >> the changes to xwin driver; however, I was not able to test tkwin, qt, or >> cairo. I will provide an update to wingcc separately. I didn’t touch the >> wxwidgets driver. >> >> I didn’t have to touch the drivers that do not need the wait function >> pointer (e.g. postscript). The way the driver dispatch table is >> initialized, I was able to set all the function pointers to NULL in the >> plcore.c file. The drivers set each member individually, thus the wait >> function pointer is left NULL. >> >> The xwin driver occasionally misses some resizes, but I think that is some >> quirky behavior that existed prior to 5.10. >> >> If someone could test the tkwin, qt, and cairo (specifically xcairo) that >> would be greatly appreciated. >> >> >> <0001-Fix-to-the-multiple-keypress-bug-on-page-advance.patch> >> >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> Plplot-devel mailing list >> Plp...@li... >> https://lists.sourceforge.net/lists/listinfo/plplot-devel >> >> >> >> ------------------------------------------------------------------------------ >> >> _______________________________________________ >> Plplot-devel mailing list >> Plp...@li... >> https://lists.sourceforge.net/lists/listinfo/plplot-devel >> |
From: Phil R. <p.d...@gm...> - 2015-06-16 21:27:56
|
Okay, so it sounds like there is basically no change for drivers that are waiting to be updated. If you need me to have a look at the patches I will try to do so tomorrow. Phil -----Original Message----- From: "Jim Dishaw" <ji...@di...> Sent: 16/06/2015 16:11 To: "Phil Rosenberg" <p.d...@gm...> Cc: "Alan W. Irwin" <ir...@be...>; "PLplot development list" <Plp...@li...>; "Andrew Ross" <and...@us...> Subject: Re: [Plplot-devel] Bug fix to plbuf.c > On Jun 16, 2015, at 4:58 AM, Phil Rosenberg <p.d...@gm...> wrote: > > So how should we go about applying all this? Will we break some > drivers when it is applied? If so do we need to have all drivers > prepared so that we can apply all the fixes together? > All function pointer are set to NULL during the dispatch table initialization, thus the wait pointer remains NULL if a device does not set it. The plP_wait() function checks nopause and for a non-NULL wait function pointer before calling the wait function pointer. Thus no action is taken if a driver does not support the wait function. For non-interactive drivers, no further action is required because they don’t wait for input. For interactive drivers, the key factor is if the driver uses plRemakePlot(). If it does, it will need to provide a wait function otherwise multiple keypresses will be required after a resize. If it does not use plRemakePlot(), the change is not critical because having a wait for input in the driver’s EOP handler will not break anything. The driver should be updated to keep the internal API consistent. For example, the tek driver is in an unsupported state and there is no need to update it. The Qt driver, on the other hand, should be updated to stay consistent with the internal API even though it does not use plRemakePlot(). If during testing we discover Qt behaves badly with this change, we can leave the changes out until a later release. > Phil > > On 16 June 2015 at 02:55, Jim Dishaw <ji...@di...> wrote: >> And here is the wingcc patch series. I tested with cygwin and msvc and did >> not have the problem with multiple keypresses. The wingcc driver does seem >> to have a problem when the window is made small. It cuts off part of the >> plot. >> >> >> >> >> On Jun 15, 2015, at 12:24 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 14, 2015, at 1:40 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 14, 2015, at 5:19 AM, Alan W. Irwin <ir...@be...> >> wrote: >> >> On 2015-06-13 22:29-0400 Jim Dishaw wrote: >> >> >> On Jun 13, 2015, at 2:11 PM, Jim Dishaw <ji...@di...> wrote: >> >> >> On Jun 13, 2015, at 1:13 PM, Phil Rosenberg <p.d...@gm... >> <mailto:p.d...@gm...>> wrote: >> >> Hmmm >> Then hmmmm some more >> Followed by an ummm or two. >> >> As you say Jim, clearly things are more complicated than I realised. I'm not >> sure what the requirements really are here now. Now you have brought this up >> I can imagine the issues associated with entering the message loop after an >> eop. >> >> In terms of dealing with it, I'm not at my computer right now, but I think >> probably a bop_forced function which forces a bop irrespective of where we >> are in the page cycle would work. But I'm not sure if that might have some >> potential subtle effects elsewhere so it is a bit of a hack really. Perhaps >> instead it would be better to assess what is being done in a bop and ensure >> those actions end up in the buffer so a bop event is not required. >> >> >> I think that is a good approach for reasons beyond this issue, but a BOP and >> EOP of some type is still required to support the drivers adequately (e.g >> double buffering, printing from the windows API). >> >> The solution I'm going to test is one where the driver does not enter into a >> wait for input state on a redraw event from the callback. >> >> >> I came up with two solutions, one of which I tested. >> >> Solution 1 (Tested) >> >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so >> now an EOP is written to the buffer. This will cause an EOP to be generated >> when the plot buffer is replayed. I like the symmetry of having an EOP in >> the buffer to match the BOP. >> >> Drivers will be responsible for determining if a “wait for user input” >> should be performed during an EOP. For the wingdi driver I track the state >> of a “page_state” that changes outside of the BOP/EOP calls. >> >> This solution feels a bit kludgy, but it works. >> >> Solution 2 >> >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so >> now an EOP is written to the buffer. This will cause an EOP to be generated >> when the plot buffer is replayed. >> >> Add a new driver function to the dispatch table (e.g. pl_wait) that calls >> the "wait for user input between pages" function. >> >> Remove from all drivers the “wait for input” from the EOP handler. >> >> The PLplot core library will call the wait for input between pages function >> (if not null). >> >> This solution strikes me as a more elegant solution because the logic for >> deciding about the wait for input is one spot. >> >> >> Regardless of the solution, some changes to the drivers will be >> >> required. Solution 2 will touch all the drivers (at a minimum to add >> a NULL to the dispatch table). Solution 1 adds code to the drivers >> while solution 2 removes some code from the drivers. >> >> Hi Jim: >> >> Thanks for your effort working through possible solutions to the >> problem. >> >> @Phil and Andrew: >> >> Assuming you guys agree with Jim's analysis, do you prefer solution 1 >> or 2? Elegant and "removes some code from the drivers" (i.e., >> solution 2) seems like the better choice to me from an overview >> perspective, but I would be happy to go along with whatever you all >> decide since I do not have the driver expertise you guys have. >> >> Assuming whatever solution is decided upon here can be implemented >> in a reasonably timely way, then I think we should be able to fit >> it into the forthcoming bug-fix release since this is obviously a >> bug fix, albeit a rather intrusive one. >> >> With regard to timing for that release there is still quite a bit I >> plan to do to work through the bugs in the build system revealed by >> previous tests on Linux and Arjen's current tests on the Cygwin >> platform. And the new tarball report that is automatically collected >> by scripts/comprehensive_test.sh should make reporting of tests and >> figuring out build-system issues from those reports _much_ easier. >> Which likely means this release will be tested on a lot more platforms >> than the last one and a significant amount of time will be required to >> deal with any build system bugs that are turned up by such tests. So >> my current guestimate is the forthcoming release is likely more than a >> month away (although hopefully not longer than two months away). >> >> >> Attached is a patch series that implements the second solution. I tested >> the changes to xwin driver; however, I was not able to test tkwin, qt, or >> cairo. I will provide an update to wingcc separately. I didn’t touch the >> wxwidgets driver. >> >> I didn’t have to touch the drivers that do not need the wait function >> pointer (e.g. postscript). The way the driver dispatch table is >> initialized, I was able to set all the function pointers to NULL in the >> plcore.c file. The drivers set each member individually, thus the wait >> function pointer is left NULL. >> >> The xwin driver occasionally misses some resizes, but I think that is some >> quirky behavior that existed prior to 5.10. >> >> If someone could test the tkwin, qt, and cairo (specifically xcairo) that >> would be greatly appreciated. >> >> >> <0001-Fix-to-the-multiple-keypress-bug-on-page-advance.patch> >> >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> Plplot-devel mailing list >> Plp...@li... >> https://lists.sourceforge.net/lists/listinfo/plplot-devel >> >> >> >> ------------------------------------------------------------------------------ >> >> _______________________________________________ >> Plplot-devel mailing list >> Plp...@li... >> https://lists.sourceforge.net/lists/listinfo/plplot-devel >> |
From: Jim D. <ji...@di...> - 2015-06-16 21:31:56
|
> On Jun 16, 2015, at 5:27 PM, Phil Rosenberg <p.d...@gm...> wrote: > > Okay, so it sounds like there is basically no change for drivers that are waiting to be updated. > If you need me to have a look at the patches I will try to do so tomorrow. The wxWidgets driver will need to be updated to work correctly because it uses the plot buffer. Without an updated, it will need an extra keypress after a resize event. Extra eyes on the patches is appreciated. Thanks. > > Phil > From: Jim Dishaw <mailto:ji...@di...> > Sent: 16/06/2015 16:11 > To: Phil Rosenberg <mailto:p.d...@gm...> > Cc: Alan W. Irwin <mailto:ir...@be...>; PLplot development list <mailto:Plp...@li...>; Andrew Ross <mailto:and...@us...> > Subject: Re: [Plplot-devel] Bug fix to plbuf.c > > > > On Jun 16, 2015, at 4:58 AM, Phil Rosenberg <p.d...@gm...> wrote: > > > > So how should we go about applying all this? Will we break some > > drivers when it is applied? If so do we need to have all drivers > > prepared so that we can apply all the fixes together? > > > > All function pointer are set to NULL during the dispatch table initialization, thus the wait pointer remains NULL if a device does not set it. The plP_wait() function checks nopause and for a non-NULL wait function pointer before calling the wait function pointer. Thus no action is taken if a driver does not support the wait function. > > For non-interactive drivers, no further action is required because they don’t wait for input. > > For interactive drivers, the key factor is if the driver uses plRemakePlot(). If it does, it will need to provide a wait function otherwise multiple keypresses will be required after a resize. If it does not use plRemakePlot(), the change is not critical because having a wait for input in the driver’s EOP handler will not break anything. The driver should be updated to keep the internal API consistent. For example, the tek driver is in an unsupported state and there is no need to update it. The Qt driver, on the other hand, should be updated to stay consistent with the internal API even though it does not use plRemakePlot(). If during testing we discover Qt behaves badly with this change, we can leave the changes out until a later release. > > > Phil > > > > On 16 June 2015 at 02:55, Jim Dishaw <ji...@di...> wrote: > >> And here is the wingcc patch series. I tested with cygwin and msvc and did > >> not have the problem with multiple keypresses. The wingcc driver does seem > >> to have a problem when the window is made small. It cuts off part of the > >> plot. > >> > >> > >> > >> > >> On Jun 15, 2015, at 12:24 PM, Jim Dishaw <ji...@di...> wrote: > >> > >> > >> On Jun 14, 2015, at 1:40 PM, Jim Dishaw <ji...@di...> wrote: > >> > >> > >> On Jun 14, 2015, at 5:19 AM, Alan W. Irwin <ir...@be...> > >> wrote: > >> > >> On 2015-06-13 22:29-0400 Jim Dishaw wrote: > >> > >> > >> On Jun 13, 2015, at 2:11 PM, Jim Dishaw <ji...@di...> wrote: > >> > >> > >> On Jun 13, 2015, at 1:13 PM, Phil Rosenberg <p.d...@gm... > >> <mailto:p.d...@gm...>> wrote: > >> > >> Hmmm > >> Then hmmmm some more > >> Followed by an ummm or two. > >> > >> As you say Jim, clearly things are more complicated than I realised. I'm not > >> sure what the requirements really are here now. Now you have brought this up > >> I can imagine the issues associated with entering the message loop after an > >> eop. > >> > >> In terms of dealing with it, I'm not at my computer right now, but I think > >> probably a bop_forced function which forces a bop irrespective of where we > >> are in the page cycle would work. But I'm not sure if that might have some > >> potential subtle effects elsewhere so it is a bit of a hack really. Perhaps > >> instead it would be better to assess what is being done in a bop and ensure > >> those actions end up in the buffer so a bop event is not required. > >> > >> > >> I think that is a good approach for reasons beyond this issue, but a BOP and > >> EOP of some type is still required to support the drivers adequately (e.g > >> double buffering, printing from the windows API). > >> > >> The solution I'm going to test is one where the driver does not enter into a > >> wait for input state on a redraw event from the callback. > >> > >> > >> I came up with two solutions, one of which I tested. > >> > >> Solution 1 (Tested) > >> > >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so > >> now an EOP is written to the buffer. This will cause an EOP to be generated > >> when the plot buffer is replayed. I like the symmetry of having an EOP in > >> the buffer to match the BOP. > >> > >> Drivers will be responsible for determining if a “wait for user input” > >> should be performed during an EOP. For the wingdi driver I track the state > >> of a “page_state” that changes outside of the BOP/EOP calls. > >> > >> This solution feels a bit kludgy, but it works. > >> > >> Solution 2 > >> > >> Remove the plP_eop() call in plRemakePlot(). Add an EOP to plbuf_eop() so > >> now an EOP is written to the buffer. This will cause an EOP to be generated > >> when the plot buffer is replayed. > >> > >> Add a new driver function to the dispatch table (e.g. pl_wait) that calls > >> the "wait for user input between pages" function. > >> > >> Remove from all drivers the “wait for input” from the EOP handler. > >> > >> The PLplot core library will call the wait for input between pages function > >> (if not null). > >> > >> This solution strikes me as a more elegant solution because the logic for > >> deciding about the wait for input is one spot. > >> > >> > >> Regardless of the solution, some changes to the drivers will be > >> > >> required. Solution 2 will touch all the drivers (at a minimum to add > >> a NULL to the dispatch table). Solution 1 adds code to the drivers > >> while solution 2 removes some code from the drivers. > >> > >> Hi Jim: > >> > >> Thanks for your effort working through possible solutions to the > >> problem. > >> > >> @Phil and Andrew: > >> > >> Assuming you guys agree with Jim's analysis, do you prefer solution 1 > >> or 2? Elegant and "removes some code from the drivers" (i.e., > >> solution 2) seems like the better choice to me from an overview > >> perspective, but I would be happy to go along with whatever you all > >> decide since I do not have the driver expertise you guys have. > >> > >> Assuming whatever solution is decided upon here can be implemented > >> in a reasonably timely way, then I think we should be able to fit > >> it into the forthcoming bug-fix release since this is obviously a > >> bug fix, albeit a rather intrusive one. > >> > >> With regard to timing for that release there is still quite a bit I > >> plan to do to work through the bugs in the build system revealed by > >> previous tests on Linux and Arjen's current tests on the Cygwin > >> platform. And the new tarball report that is automatically collected > >> by scripts/comprehensive_test.sh should make reporting of tests and > >> figuring out build-system issues from those reports _much_ easier. > >> Which likely means this release will be tested on a lot more platforms > >> than the last one and a significant amount of time will be required to > >> deal with any build system bugs that are turned up by such tests. So > >> my current guestimate is the forthcoming release is likely more than a > >> month away (although hopefully not longer than two months away). > >> > >> > >> Attached is a patch series that implements the second solution. I tested > >> the changes to xwin driver; however, I was not able to test tkwin, qt, or > >> cairo. I will provide an update to wingcc separately. I didn’t touch the > >> wxwidgets driver. > >> > >> I didn’t have to touch the drivers that do not need the wait function > >> pointer (e.g. postscript). The way the driver dispatch table is > >> initialized, I was able to set all the function pointers to NULL in the > >> plcore.c file. The drivers set each member individually, thus the wait > >> function pointer is left NULL. > >> > >> The xwin driver occasionally misses some resizes, but I think that is some > >> quirky behavior that existed prior to 5.10. > >> > >> If someone could test the tkwin, qt, and cairo (specifically xcairo) that > >> would be greatly appreciated. > >> > >> > >> <0001-Fix-to-the-multiple-keypress-bug-on-page-advance.patch> > >> > >> > >> ------------------------------------------------------------------------------ > >> _______________________________________________ > >> Plplot-devel mailing list > >> Plp...@li... > >> https://lists.sourceforge.net/lists/listinfo/plplot-devel > >> > >> > >> > >> ------------------------------------------------------------------------------ > >> > >> _______________________________________________ > >> Plplot-devel mailing list > >> Plp...@li... > >> https://lists.sourceforge.net/lists/listinfo/plplot-devel > >> > |
From: Alan W. I. <ir...@be...> - 2015-06-20 07:45:20
|
On 2015-06-15 12:24-0400 Jim Dishaw wrote: > Attached is a patch series that implements the second solution. I tested the changes to xwin driver; however, I was not able to test tkwin, qt, or cairo. I will provide an update to wingcc separately. I didn’t touch the wxwidgets driver. > The xwin driver occasionally misses some resizes, but I think that is some quirky behavior that existed prior to 5.10. > If someone could test the tkwin, qt, and cairo (specifically xcairo) that would be greatly appreciated. Hi Jim: As promised I finally have had a chance to test this patch series. It applies cleanly (aside for some white space issues that are automatically cleaned up by git) to a private topic branch based on git master tip. Also, I encountered no obvious issues with builds (other than the easily fixed cairo issue below). N.B. So once you have fixed the cairo build issue please go ahead and push this result as well as your other result involving wingcc (assuming you have testing resizing with that device). Here are some specific comments with respect to resize issues still encountered with the various device drivers I tested typically using the examples/c/x08c -dev <interactive device> command for various interactive devices. * qtwidget The resizing results seem _almost_ perfect to me. No black screen, graphical and text results scaled properly no matter how much I dragged the corner of the qtwidget GUI around my desktop. However, if I drag from a large size _rapidly to a small one, there is often no response, and I had to repeat the process with a slower drag. I believe this is what you referred to above (for the xwin device) as missing some resizes. Presumably this is a long-standing issue we should address some day. Aside from this one issue, in my opinion the behavior of this device on resize events should be the paradigm for all other interactive devices. In particular, for me text scaling on resize events is the right thing to do, but from recent list traffic I am still in the process of convincing Phil on that issue for the wxwidgets device and you on that issue for the new GDI device. :-) But if either of you try qtwidget, I think those nice scaled text results should convince you to at least provide a (default) option to do text scaling on resize events for the wxwidgets and new GDI device cases. * tk This device (although it uses ugly Hershey fonts rather than system fonts like qtwidget) has perfect resizing of graphics and text. And despite maximum violence of my mouse moves, I could not get it to miss any resizes (unlike qtwidgets above). However, it is not a good candidate for a paradigm of a typical interactive device driver because of all the complex interactions with Tcl, Tk, and the xwin device. * xwin Your fix has solved the black screen issue I reported for this device. and like the tk device (which is not surprising because they share access to some xwin features) I cannot get it to miss resizes (unlike what you reported above unless I am misinterpreting what you said). However, the scaling and centering of graphics and text is out of synch with the resize event, i.e., it reflects the size of the last GUI rather than present GUI as you should be able to prove by making a single large change in GUI size and releasing the mouse without further movement. I presume the problem here is the xwin device is updating the scale and size of the result using data collected prior to the resizing event rather than at the correct time after that event so the results always reflect those incorrect prior GUI sizes. If you can quickly figure out a fix for this specific xwin bug (presumably in existence for a long time), please push it, but otherwise we can continue to live with it. * tkwin The only way you can exercise this "device" is with the wish technique for the runAllDemos example documented in examples/tk/README.tkdemos and which is also run automatically (and with all dependencies satisfied) using make test_wish_runAllDemos There has been something wrong with this example for a while now (it warns about, e.g., x08 invalid command name when you hit the button for the 8th example). So although the GUI exists and you can resize it, the plot window subset of that GUI remains blank so you cannot currently see if there are any issues with plot resizing with the tkwin "device". * xcairo Your change produces the following build error /home/software/plplot/HEAD/plplot.git/drivers/cairo.c: In function ‘plD_wait_xcairo’: /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2149:5: error: ‘event_mask’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2149:5: note: each undeclared identifier is reported only once for each function it appears in /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2154:41: error: ‘event’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:13: error: ‘number_chars’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:65: error: ‘event_string’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:84: error: ‘keysym’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:93: error: ‘cs’ undeclared (first use in this function) /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2177:13: error: ‘expose’ undeclared (first use in this function) make[3]: *** [drivers/CMakeFiles/cairo.dir/cairo.c.o] Error 1 make[2]: *** [drivers/CMakeFiles/cairo.dir/all] Error 2 make[1]: *** [drivers/CMakeFiles/cairo.dir/rule] Error 2 make: *** [cairo] Error 2 I believe the problem is you left your declarations in the wrong function. This patch fixes that issue: diff --git a/drivers/cairo.c b/drivers/cairo.c index c3e949a..c7f2ba9 100644 --- a/drivers/cairo.c +++ b/drivers/cairo.c @@ -2084,13 +2084,6 @@ void plD_bop_xcairo( PLStream *pls ) void plD_eop_xcairo( PLStream *pls ) { - int number_chars; - long event_mask; - char event_string[10]; - KeySym keysym; - XComposeStatus cs; - XEvent event; - XExposeEvent *expose; PLCairo *aStream; aStream = (PLCairo *) pls->dev; @@ -2139,6 +2132,13 @@ void plD_tidy_xcairo( PLStream *pls ) void plD_wait_xcairo( PLStream *pls ) { + int number_chars; + long event_mask; + char event_string[10]; + KeySym keysym; + XComposeStatus cs; + XEvent event; + XExposeEvent *expose; PLCairo *aStream; aStream = (PLCairo *) pls->dev; After that change, the cairo device built without issues. However, after applying the above patch to your work, the plD_eop_xcairo function is reduced just to void plD_eop_xcairo( PLStream *pls ) { PLCairo *aStream; aStream = (PLCairo *) pls->dev; // Blit the offscreen image to the X window. blit_to_x( pls, 0.0, 0.0, pls->xlength, pls->ylength ); if ( aStream->xdrawable_mode ) return; } and I assume you want to do more than the default return at the end of the function when that last if condition is NOT satisfied. The most important problem with the response to resize events for xcairo is that it is essentially nonexistent. The same plot with exactly the same size gets replotted so either it is smaller than the GUI window if you have resized larger or else it is too large for the GUI window (and clipped off at the edges of the GUI) if you have resized smaller. If you can find an easy fix for this apparently long-standing issue, that would be great, but otherwise we can just live with it until some regular user of xcairo gets fed up with this poor resize result. * wincairo This is a windows equivalent of xcairo so I was unable to test it, but presumably it needs work to deal with resizing events. I am not sure whether wincairo will be available on Cygwin, but it should be available on MSVC if you download and install GTK+ for Windows on that platform. *ntk This is an important Tk device because it is completely independent of X (and, for example, therefore executes much faster than the tk device on Cygwin which does depend on X). The ntk device obviously needs work to deal properly with resizing events since, for example, there is the same lack of resizing capability as for xcairo. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Jim D. <ji...@di...> - 2015-06-22 03:50:25
|
I fixed the compilation error on cairo.c and I am working on a fix to cairo so that it actually resizes the plot (at least for xcairo, not sure when I can fix wincairo) when the window changes size. I will also take a look at xwin when I get a chance. I think the problem has to do with the sequencing of the events. I pushed the changes to the repository. > On Jun 20, 2015, at 3:45 AM, Alan W. Irwin <ir...@be...> wrote: > > On 2015-06-15 12:24-0400 Jim Dishaw wrote: > >> Attached is a patch series that implements the second solution. I > tested the changes to xwin driver; however, I was not able to test > tkwin, qt, or cairo. I will provide an update to wingcc separately. I didn’t touch the wxwidgets driver. > >> The xwin driver occasionally misses some resizes, but I think that is > some quirky behavior that existed prior to 5.10. > >> If someone could test the tkwin, qt, and cairo (specifically xcairo) > that would be greatly appreciated. > > Hi Jim: > > As promised I finally have had a chance to test this patch series. It > applies cleanly (aside for some white space issues that are > automatically cleaned up by git) to a private topic branch based on > git master tip. Also, I encountered no obvious issues with builds > (other than the easily fixed cairo issue below). > > N.B. So once you have fixed the cairo build issue please go ahead and > push this result as well as your other result involving wingcc > (assuming you have testing resizing with that device). > > Here are some specific comments with respect to resize issues still > encountered with the various device drivers I tested typically using the > > examples/c/x08c -dev <interactive device> > > command for various interactive devices. > > * qtwidget > > The resizing results seem _almost_ perfect to me. No black screen, > graphical and text results scaled properly no matter how much I > dragged the corner of the qtwidget GUI around my desktop. However, if > I drag from a large size _rapidly to a small one, there is often no > response, and I had to repeat the process with a slower drag. I > believe this is what you referred to above (for the xwin device) as > missing some resizes. Presumably this is a long-standing issue we > should address some day. Aside from this one issue, in my opinion the > behavior of this device on resize events should be the paradigm for > all other interactive devices. > > In particular, for me text scaling on resize events is the right thing > to do, but from recent list traffic I am still in the process of > convincing Phil on that issue for the wxwidgets device and you on that > issue for the new GDI device. :-) But if either of you try qtwidget, > I think those nice scaled text results should convince you to at least > provide a (default) option to do text scaling on resize events for the > wxwidgets and new GDI device cases. > > * tk > > This device (although it uses ugly Hershey fonts rather than system > fonts like qtwidget) has perfect resizing of graphics and text. And > despite maximum violence of my mouse moves, I could not get it to miss > any resizes (unlike qtwidgets above). However, it is not a good > candidate for a paradigm of a typical interactive device driver > because of all the complex interactions with Tcl, Tk, and the xwin > device. > > * xwin > > Your fix has solved the black screen issue I reported for this device. > and like the tk device (which is not surprising because they share > access to some xwin features) I cannot get it to miss resizes (unlike > what you reported above unless I am misinterpreting what you said). > However, the scaling and centering of graphics and text is out of > synch with the resize event, i.e., it reflects the size of the last > GUI rather than present GUI as you should be able to prove by making a > single large change in GUI size and releasing the mouse without > further movement. I presume the problem here is the xwin device is > updating the scale and size of the result using data collected prior > to the resizing event rather than at the correct time after that event > so the results always reflect those incorrect prior GUI sizes. If you > can quickly figure out a fix for this specific xwin bug (presumably in > existence for a long time), please push it, but otherwise we can > continue to live with it. > > * tkwin > The only way you can exercise this "device" is with the wish technique > for the runAllDemos example documented in examples/tk/README.tkdemos > and which is also run automatically (and with all dependencies > satisfied) using > > make test_wish_runAllDemos > > There has been something wrong with this example for a while now (it > warns about, e.g., > > x08 invalid command name > > when you hit the button for the 8th example). So although the GUI > exists and you can resize it, the plot window subset of that GUI > remains blank so you cannot currently see if there are any issues with > plot resizing with the tkwin "device". > > * xcairo > > Your change produces the following build error > > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c: In function ‘plD_wait_xcairo’: > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2149:5: error: ‘event_mask’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2149:5: note: each undeclared identifier is reported only once for each function it appears in > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2154:41: error: ‘event’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:13: error: ‘number_chars’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:65: error: ‘event_string’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:84: error: ‘keysym’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2158:93: error: ‘cs’ undeclared (first use in this function) > /home/software/plplot/HEAD/plplot.git/drivers/cairo.c:2177:13: error: ‘expose’ undeclared (first use in this function) > make[3]: *** [drivers/CMakeFiles/cairo.dir/cairo.c.o] Error 1 > make[2]: *** [drivers/CMakeFiles/cairo.dir/all] Error 2 > make[1]: *** [drivers/CMakeFiles/cairo.dir/rule] Error 2 > make: *** [cairo] Error 2 > > I believe the problem is you left your declarations in the wrong function. This > patch fixes that issue: > diff --git a/drivers/cairo.c b/drivers/cairo.c > index c3e949a..c7f2ba9 100644 > --- a/drivers/cairo.c > +++ b/drivers/cairo.c > @@ -2084,13 +2084,6 @@ void plD_bop_xcairo( PLStream *pls ) > > void plD_eop_xcairo( PLStream *pls ) > { > - int number_chars; > - long event_mask; > - char event_string[10]; > - KeySym keysym; > - XComposeStatus cs; > - XEvent event; > - XExposeEvent *expose; > PLCairo *aStream; > > aStream = (PLCairo *) pls->dev; > @@ -2139,6 +2132,13 @@ void plD_tidy_xcairo( PLStream *pls ) > > void plD_wait_xcairo( PLStream *pls ) > { > + int number_chars; > + long event_mask; > + char event_string[10]; > + KeySym keysym; > + XComposeStatus cs; > + XEvent event; > + XExposeEvent *expose; > PLCairo *aStream; > > aStream = (PLCairo *) pls->dev; > > After that change, the cairo device built without issues. However, > after applying the above patch to your work, the plD_eop_xcairo function is reduced > just to > > void plD_eop_xcairo( PLStream *pls ) > { > PLCairo *aStream; > > aStream = (PLCairo *) pls->dev; > > // Blit the offscreen image to the X window. > blit_to_x( pls, 0.0, 0.0, pls->xlength, pls->ylength ); > > if ( aStream->xdrawable_mode ) > return; > } > > and I assume you want to do more than the default return at the end of > the function when that last if condition is NOT satisfied. > > The most important problem with the response to resize events for > xcairo is that it is essentially nonexistent. The same plot with > exactly the same size gets replotted so either it is smaller than the > GUI window if you have resized larger or else it is too large for the > GUI window (and clipped off at the edges of the GUI) if you have > resized smaller. If you can find an easy fix for this apparently > long-standing issue, that would be great, but otherwise we can just > live with it until some regular user of xcairo gets fed up with this > poor resize result. > > * wincairo > > This is a windows equivalent of xcairo so I was unable to test it, but > presumably it needs work to deal with resizing events. I am not sure > whether wincairo will be available on Cygwin, but it should be available > on MSVC if you download and install GTK+ for Windows on that platform. > > *ntk > > This is an important Tk device because it is completely independent of > X (and, for example, therefore executes much faster than the tk device > on Cygwin which does depend on X). The ntk device obviously needs > work to deal properly with resizing events since, for example, there > is the same lack of resizing capability as for xcairo. > > Alan > __________________________ > Alan W. Irwin > > Astronomical research affiliation with Department of Physics and Astronomy, > University of Victoria (astrowww.phys.uvic.ca). > > Programming affiliations with the FreeEOS equation-of-state > implementation for stellar interiors (freeeos.sf.net); the Time > Ephemerides project (timeephem.sf.net); PLplot scientific plotting > software package (plplot.sf.net); the libLASi project > (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); > and the Linux Brochure Project (lbproject.sf.net). > __________________________ > > Linux-powered Science > __________________________ |
From: Alan W. I. <ir...@be...> - 2015-06-22 18:05:35
|
On 2015-06-21 23:50-0400 Jim Dishaw wrote: > I fixed the compilation error on cairo.c and I am working on a fix to cairo so that it actually resizes the plot (at least for xcairo, not sure when I can fix wincairo) when the window changes size. > > I will also take a look at xwin when I get a chance. I think the problem has to do with the sequencing of the events. > > I pushed the changes to the repository. Hi Jim: Thanks very much for that work, and I am looking forward to your further work on cairo.c and xwin.c. There are three minor issues with your current push which you will likely want to address for your future pushes. 1. The results needed to be styled which I did (8025ebe). I emphasize that is no trouble for me, and I am willing to do that indefinitely, but it does mean your rebasing in future can get complicated if any of those white space styling changes I make are in an area of the file you are working on. Therefore, to avoid that potential issue I suggest you contact Phil to figure out how to style your further commits on Windows (and me if you want to style your further commits on your OS X box). 2. The commit message had no separate short initial line summarizing the commit. That style of commit message is recommended in both <http://who-t.blogspot.be/2009/12/on-commit-messages.html> and [Pro Git Book](http://git-scm.com/book). The reason for that recommendation is many git tools make use of the first separate line of the commit message to help identify commits for humans. 3. When I styled your changes, my git software recognized there was a permissions issue on one of the files you changed, i.e., mode change 100755 => 100644 drivers/wingcc.c The 755 permission bits mean rwxr-xr-x for user, group, and other, and the 644 permission bits mean rw-r--r-- for user, group, and other, where r is read permission, w is write permission, and x is execute permission. I double checked and the permissions for this file were rw-r--r-- for 5.11.0. Therefore, as a result of something you did or your git application did, the permissions on drivers/wingcc.c were incorrectly updated to execute for all of user, group, and other by your editing, commit, or push. The interesting thing is every other file you edited and pushed had no such permission problems. So can you think of anything special you did for drivers/wingcc.c that may have caused this permissions issue? Note, as far as I am aware we (including Arjen and Phil on Windows and me on Linux) are all just using the default configuration of git on our various platforms, and this is the first time a permissions issue has occurred. I think the reason permission bits are normally not an issue with git is it is quite smart about default permissions for various file types. In particular files with a ".c" suffix should never have an execute permission assigned by default with git unless the user does something specific to override that default. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Jim D. <ji...@di...> - 2015-06-22 18:34:35
|
> On Jun 22, 2015, at 1:50 PM, Alan W. Irwin <ir...@be...> wrote: > >> On 2015-06-21 23:50-0400 Jim Dishaw wrote: >> >> I fixed the compilation error on cairo.c and I am working on a fix to cairo so that it actually resizes the plot (at least for xcairo, not sure when I can fix wincairo) when the window changes size. >> >> I will also take a look at xwin when I get a chance. I think the problem has to do with the sequencing of the events. >> >> I pushed the changes to the repository. > > Hi Jim: > > Thanks very much for that work, and I am looking forward to your further > work on cairo.c and xwin.c. > > There are three minor issues with your current push which you will > likely want to address for your future pushes. > > 1. The results needed to be styled which I did (8025ebe). I emphasize > that is no trouble for me, and I am willing to do that indefinitely, > but it does mean your rebasing in future can get complicated if any of > those white space styling changes I make are in an area of the file > you are working on. Therefore, to avoid that potential issue I > suggest you contact Phil to figure out how to style your further > commits on Windows (and me if you want to style your further commits > on your OS X box). I ran scripts/style_sources.sh on my Mac. I do all my commits from that machine. I see it list the files that I have changed. Perhaps it is silently failing? I will try to single-step the script. > > 2. The commit message had no separate short initial line summarizing > the commit. That style of commit message is recommended in both > <http://who-t.blogspot.be/2009/12/on-commit-messages.html> and [Pro > Git Book](http://git-scm.com/book). The reason for that recommendation > is many git tools make use of the first separate line of the commit > message to help identify commits for humans. The browse tool shows my commit message. Do I need to make it more descriptive? > > 3. When I styled your changes, my git software recognized there was > a permissions issue on one of the files you changed, i.e., > > mode change 100755 => 100644 drivers/wingcc.c > I think that is an artifact of moving the file from the windows machine to the mac. > Note, as far as I am aware we (including Arjen and Phil on Windows and > me on Linux) are all just using the default configuration of git on > our various platforms, and this is the first time a permissions issue > has occurred. I think the reason permission bits are normally not an > issue with git is it is quite smart about default permissions for > various file types. In particular files with a ".c" suffix should > never have an execute permission assigned by default with git unless > the user does something specific to override that default. > > Alan > __________________________ > Alan W. Irwin > > Astronomical research affiliation with Department of Physics and Astronomy, > University of Victoria (astrowww.phys.uvic.ca). > > Programming affiliations with the FreeEOS equation-of-state > implementation for stellar interiors (freeeos.sf.net); the Time > Ephemerides project (timeephem.sf.net); PLplot scientific plotting > software package (plplot.sf.net); the libLASi project > (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); > and the Linux Brochure Project (lbproject.sf.net). > __________________________ > > Linux-powered Science > __________________________ |
From: Alan W. I. <ir...@be...> - 2015-06-22 19:40:45
|
On 2015-06-22 14:34-0400 Jim Dishaw wrote: > > >> On Jun 22, 2015, at 1:50 PM, Alan W. Irwin <ir...@be...> wrote: >> >>> On 2015-06-21 23:50-0400 Jim Dishaw wrote: >>> >>> I fixed the compilation error on cairo.c and I am working on a fix to cairo so that it actually resizes the plot (at least for xcairo, not sure when I can fix wincairo) when the window changes size. >>> >>> I will also take a look at xwin when I get a chance. I think the problem has to do with the sequencing of the events. >>> >>> I pushed the changes to the repository. >> >> Hi Jim: >> >> Thanks very much for that work, and I am looking forward to your further >> work on cairo.c and xwin.c. >> >> There are three minor issues with your current push which you will >> likely want to address for your future pushes. >> >> 1. The results needed to be styled which I did (8025ebe). I emphasize >> that is no trouble for me, and I am willing to do that indefinitely, >> but it does mean your rebasing in future can get complicated if any of >> those white space styling changes I make are in an area of the file >> you are working on. Therefore, to avoid that potential issue I >> suggest you contact Phil to figure out how to style your further >> commits on Windows (and me if you want to style your further commits >> on your OS X box). > > I ran scripts/style_sources.sh on my Mac. I do all my commits from that machine. I see it list the files that I have changed. Perhaps it is silently failing? I will try to single-step the script. Try scripts/style_sources.sh --help to find documentation of all options for the script. With no arguments that script simple lists the files that would be changed (which is likely the results you saw) without actually changing any of the files. With the --apply argument, and if you answer the further question with "yes" it will do the wholesale style change to all files that it lists. I designed the script this way (opt-in twice) because it potentially is extremely intrusive. So you might also want to try the --diff -au option to see what the style changes would be. In the last couple of years of experience with this script, I have rarely seen any problems introduced by it, but there has been at least one of those in the last 6 months due to an uncrustify bug so I always try to make it a habit to test styled results rather than results before styling. > >> >> 2. The commit message had no separate short initial line summarizing >> the commit. That style of commit message is recommended in both >> <http://who-t.blogspot.be/2009/12/on-commit-messages.html> and [Pro >> Git Book](http://git-scm.com/book). The reason for that recommendation >> is many git tools make use of the first separate line of the commit >> message to help identify commits for humans. > > The browse tool shows my commit message. Do I need to make it more descriptive? Oh. Sorry. That style commit message style is fine. I just completely misread the ascii markdown format feed data although now it is obvious in those ascii results (and also their html equivalent) that you have followed the recommended style with an initial separate summary line. > >> >> 3. When I styled your changes, my git software recognized there was >> a permissions issue on one of the files you changed, i.e., >> >> mode change 100755 => 100644 drivers/wingcc.c >> > > I think that is an artifact of moving the file from the windows machine to the mac. OK. Glad that is explained, and I believe that terminates this topic. However, on the different topic of incorrect permissions set by commits other than your recent ones, I have decided to follow up by checking all our current file permissions to make sure nothing else like this has crept in from any of our historical commits. So from find $(ls | grep "/") -type f -a '!' -name "*.sh*" -a '!' \ -perm u=rw,go=r |less I find 183 files in our source tree that are not named "*.sh" and which do not have rw-r--r-- permissions. Some of those have obviously wrong permissions (such as one of our README files). Tonight or tomorrow I plan to improve those find stanzas to eliminate files that should have permissions different than rw-r--r-- in any case (such as the *.sh* files I have already eliminated from consideration), and then go through the remaining files that are found and manually fix any bad permissions that turn up. Alan __________________________ Alan W. Irwin Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |