<div dir="ltr">Hi Laura,<div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 26, 2016 at 7:00 AM, Laura Perez Cerrato <span dir="ltr"><<a href="mailto:lauraperezcerrato@gmail.com" target="_blank">lauraperezcerrato@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br>Elliot, Dave,<br>
<br>
Thanks for your feedback! I didn't realise the differences between the<br>
coding practices of JPEGReadWriter2Plugin and other plugins since I<br>
didn't look at the code of the others. I'll try to get the code of<br>
JPEGReadWriter2Plugin to bear more resemblance to the rest of the<br>
codebase. I've taken a brief look at RePlugin's code and the way the C<br>
code should be placed into external files is still a bit confusing to<br>
me. Any guidance in this aspect would be greatly appreciated.<br></blockquote><div><br></div><div>So the scheme is</div><div>a) the generated C source for the Smalltalk plugin class is generated in src/plugins/PluginName/PluginName.c, and it includes a PluginName.h</div><div>b) the PluginName.h lives in platforms/Cross/plugins/PluginName/PluginName.h. It should define the functions and types exported by any C sport for the PluginName.c file to use.</div><div>c) if the C support code is cross-platform it should live in platforms/Cross/plugins/PluginName/sqPluginName.c, which also includes PluginName.c. That's the case here, so refactor the bodies of all plugin methods so that the C code tat is in cCode: '...here...' strings gets moved to functions in platforms/Cross/plugins/PluginName/sqPluginName.c. These functions can be called anything you like but there's a convention to prepend "sq" or "io". For example in the FilePlugin names like sqFileFlush. "io" tends to be for more GUI and OS related functionality.</div><div>d) if the C code is not cross p;at form, or part of it is not cross platform, that code goes in platforms/platformName/plugins/PluginName/sqPlatPluginName.c, e.g. platforms/win32/plugins/UUIDPlugin/sqWin32UUID.c</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
As I mentioned earlier, I've been working on handling non-even width<br>
16-bit Forms and non-0 modulo 4 width 8 bit Forms. Here are two<br>
changesets with the implementation of that feature.<br></blockquote><div><br></div><div>Thanks!!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Cheers!<br>
-Laura Perez Cerrato<br>
<br>
<br>
On 19 May 2016 at 23:05, David T. Lewis <<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>> wrote:<br>
><br>
> Hi Laura,<br>
><br>
> Thanks very much for posting this. I took a quick look at it and will<br>
> try to follow up later.<br>
><br>
> To clarify one point: Eliot is using the word "horrible" to refer to<br>
> some coding practices in the existing JPEGReadWriter2Plugin, and it does<br>
> not mean that your patches are "horrible".<br>
><br>
><br>
> Eliot,<br>
><br>
> Indeed, those giant blocks of #cCode are bad, and somebody (tm) should<br>
> do something about it. It may not be obvious from looking at the change<br>
> set, but Laura's updates are are patches against that existing code.<br>
> So your suggestion is good - either rewrite those portions in proper<br>
> Smalltalk slang, or just move them out to C code in the platforms support.<br>
><br>
> I think that this is probably beyond the scope of what Laura is trying to<br>
> do here, although I would encourage anyone with an interest in improving<br>
> the plugins to take a look at reworking the #cCode portions of<br>
> JPEGReadWriter2Plugin.<br>
><br>
> Dave<br>
><br>
><br>
><br>
><br>
> On Thu, May 19, 2016 at 04:29:15PM -0700, Eliot Miranda wrote:<br>
>><br>
>> Hi Laura,<br>
>><br>
>> this isn't your fault, but looking at the body of the plugin methods,<br>
>> the code is horrible. It's essentially 100% C. The code should really be<br>
>> written in C files, not in Smalltalk methods, following the pattern of<br>
>> separating plugin code into a Smalltalk plugin larger calling platform<br>
>> functions written in a C layer. For example, look at the REPlugin which<br>
>> does regular expressions. The source to the machinery is in<br>
>> platforms/Cross/plugins/RePlugin. The plugin methods, e.g.<br>
>> REPlugin>>primPCREExecfromto, do marshalling of Smalltalk objects into C<br>
>> parameters and then invoke function implemented in<br>
>> platforms/Cross/plugins/RePlugin, e.g. pcre_exec. This approach means that<br>
>> there's onlyas much cCode:inSmalltalk: as necessary. This is far nicer<br>
>> code to read and fix.<br>
>><br>
>> Do you have energy to rewrite the JPEG plain code to follow this pattern?<br>
>> If so, let me encourage you. It will improve the code hugely.<br>
>><br>
>> On Thu, May 19, 2016 at 7:06 AM, Laura Perez Cerrato <<br>
>> <a href="mailto:lauraperezcerrato@gmail.com">lauraperezcerrato@gmail.com</a>> wrote:<br>
>><br>
>> ><br>
>> > Elliot, Dave,<br>
>> ><br>
>> > Thanks for your answers! Here go both the changesets requested. The<br>
>> > JPEGReadWriter2Plugin changeset was generated using a Squeak Spur<br>
>> > trunk image, whilst the Graphics changeset was generated using the<br>
>> > latest stable version of Squeak non-Spur (4.6-15102). However, It<br>
>> > should also work well with a Squeak Spur trunk image, though I haven't<br>
>> > tested it's functioning there thoroughly. I haven't yet implemented a<br>
>> > graceful way for the new primitive to fail if the new version of the<br>
>> > plugin is absent, so keep that in mind.<br>
>> ><br>
>> > I'm currently working on handling non-even width 16-bit Forms and<br>
>> > non-0 modulo 4 width 8 bit Forms, so I'll hopefully send that to you<br>
>> > too within this week.<br>
>> ><br>
>> > -Laura Perez Cerrato<br>
>> ><br>
>> ><br>
>> > On 18 May 2016 at 22:00, David T. Lewis <<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>> wrote:<br>
>> > ><br>
>> > > On Wed, May 18, 2016 at 03:56:11PM -0700, Eliot Miranda wrote:<br>
>> > >><br>
>> > >> Hi Laura,<br>
>> > >><br>
>> > >> > On May 18, 2016, at 1:04 PM, Laura Perez Cerrato <<br>
>> > <a href="mailto:lauraperezcerrato@gmail.com">lauraperezcerrato@gmail.com</a>> wrote:<br>
>> > >> ><br>
>> > >> ><br>
>> > >> > Hello, everyone,<br>
>> > >> ><br>
>> > >> > During the past few days I've been working on adding support for<br>
>> > >> > reading and writing reading and writing JPEGs to/from -32, -16, -8<br>
>> > >> > grayscale and 8 grayscale bit deep forms to JPEGReadWriter2Plugin for<br>
>> > >> > the Squeak Cog V3 VM. Just chiming in to tell you that the work is<br>
>> > >> > done and I'd like to share it with you all. :) I believe this changes<br>
>> > >> > should work on Spur VMs too, but haven't tested thoroughly enough to<br>
>> > >> > ensure that.<br>
>> > >> ><br>
>> > >> > How do you handle contributions to the codebase? I assume I'd have to<br>
>> > >> > upload the corresponding changeset to the repository but I'm not sure<br>
>> > >> > if there's anything I should do before.<br>
>> > >> ><br>
>> > >> > I have also been working on a few changes to the latest non-Spur<br>
>> > >> > version of Squeak in order to take advantage of the changes to the<br>
>> > >> > plugin but, since I added a primitive to it and these changes use it,<br>
>> > >> > I think it's prudent to ask you too how those contributions are<br>
>> > >> > handled. Should I refer to the Squeak-dev mailing list?<br>
>> > >><br>
>> > >> This is indeed a problem. Talking for myself, until VMs have been<br>
>> > built that include the code and are generally available one can't really<br>
>> > deploy the behaviour to trunk. But again waiting for the next release<br>
>> > feels far too slow.<br>
>> > >><br>
>> > >> One approach is to make the behaviour optional, depending on what the<br>
>> > VM provides, eg checking for primitive failure or a plugin version number,<br>
>> > or simply making the code fail gracefully.<br>
>> > >><br>
>> > >> Another approach is to provide the functionality in an extension<br>
>> > package, and merging that extension into trunk at the next release.<br>
>> > >><br>
>> > >> Other suggestions folks?<br>
>> > >><br>
>> > ><br>
>> > > We should take a look at the image side code too. Laura, perhaps you can<br>
>> > > post a change set for that also?<br>
>> > ><br>
>> > > Very often a new optional primitive can be handled simply by providing<br>
>> > > some suitable fallback code. That provides a smooth transition, because<br>
>> > > the new feature is immediately available and the system continues to work<br>
>> > > for people who do not yet have the new primitive in their VM.<br>
>> > ><br>
>> > > Dave<br>
>> > ><br>
>> ><br>
>> ><br>
>><br>
>><br>
>> --<br>
>> _,,,^..^,,,_<br>
>> best, Eliot<br>
><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>