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