prisonerjohn / processing-web

0 stars 0 forks source link

[CLOSED] PImage.set() cryptic documentation #153

Open prisonerjohn opened 10 years ago

prisonerjohn commented 10 years ago

Issue by teo1978 Sunday May 26, 2013 at 01:27 GMT Originally opened as https://github.com/processing/processing-web/issues/153


http://processing.org/reference/PImage_set_.html

Syntax
img.set(x, y, c) img.set(x, y, img) Parameters
img PImage: any variable of type PImage x int: x-coordinate of the pixel y int: y-coordinate of the pixel c int: any value of the color datatype img PImage: image to draw on screen

  1. you're using the same variable name "img" for two variables
  2. "img: image to draw ON SCREEN"?!?!?!?!? doesn't make any sense. Looks like something that got copy-paste from some other function's doc.
prisonerjohn commented 10 years ago

Comment by alignedleft Sunday May 26, 2013 at 02:44 GMT


1) I believe that "img" is appearing twice here because: The first time it appears, it's referencing the object on which this method can be used (any PImage). The second time, it's referencing the PImage that is copied ("set") into the original PImage. @REAS can you confirm that's right?

2) Good catch, thanks for this. I've changed this language from:

PImage: image to draw on screen

to

PImage: image to copy into the original image

prisonerjohn commented 10 years ago

Comment by REAS Sunday May 26, 2013 at 02:50 GMT


@alignedleft You need to change this in PImage.java:

This is confusing reference because "img" is the actual name of the parameter. We should change the instance name to be different from the name of this parameter. I would change it to this:

prisonerjohn commented 10 years ago

Comment by alignedleft Sunday May 26, 2013 at 02:56 GMT


@REAS Great, thanks. Does changing the @instanceName value here have any impact outside of the "syntax" and "parameters" sections of the ref pages for PImage methods? I just want to make sure that this small change won't conflict with anything else.

Also, what would you think about rephrasing like so?

To me, "object" is more clear here than "variable", as "variable" sounds too much like "parameter". (In this case, "image" is the object, "img" is the parameter, though it is itself another object…)

prisonerjohn commented 10 years ago

Comment by REAS Sunday May 26, 2013 at 02:58 GMT


Yes, that's a good change to "object." I don't think it has any affect outside of naming the object variable on the reference page.

prisonerjohn commented 10 years ago

Comment by alignedleft Sunday May 26, 2013 at 03:04 GMT


Thanks. One more consideration: We seem to use "img" throughout the PImage method examples. Any concerns about potential confusion if we use "img" in all the examples, and "image" only for the @instanceName? For example, here is what we have for PImage.resize():

screen shot 2013-05-25 at 8 00 21 pm

I think this is OK, but just want to raise the point.

prisonerjohn commented 10 years ago

Comment by REAS Sunday May 26, 2013 at 03:08 GMT


Yes, that's an extremely good point. Let's go back to what we had before, to use "img", but keep your new description. Let's leave this open for now, but work on fixing other components of the Reference.

prisonerjohn commented 10 years ago

Comment by teo1978 Sunday May 26, 2013 at 13:40 GMT


It seems to me pretty simple: Instead of changing: Syntax: img.set(x,y,img) // WRONG to Syntax: image.set(x,y,img) //potentially confusing as alignedleft points out

Why don't you change it to Syntax: img.set(x,y,srcImage) //or whatever you prefer other than srcImage

It definitely CANNOT be left as is: img.set(x,y,img) // IMPLIES YOU COPY THE IMAGE INTO ITSELF

P.S. I'm talking about documentation only, of course

prisonerjohn commented 10 years ago

Comment by REAS Sunday May 26, 2013 at 14:57 GMT


@matteosistisette It's really not that simple. I wish it where. This reference entry is one case where it is very confusing, we all agree. But changing it affects all of the other image reference entries as well. We can't simply change the name of the parameter because that's the name in the source code. If you read PImage.java you'll see "img" as the parameter throughout.

I'm thinking we can use "im" as the object name in the reference. It's not optimal, but it's the best that I can imagine right now.

prisonerjohn commented 10 years ago

Comment by teo1978 Sunday May 26, 2013 at 15:18 GMT


Oh, wow, I see. Then, feature request for the system that automatically generates documentation: allow to remap the actual names of parameters in the code to fictitious names in documentation. Surprising that it doesn't have such a feature already.

prisonerjohn commented 10 years ago

Comment by alignedleft Sunday May 26, 2013 at 16:19 GMT


Yes, something like that would be great. Another consideration is that throughout all the PImage examples we use "img" as the PImage variable name. I think this name was originally chosen for simplicity, or perhaps because it's what's most common in the source. So whether we move to "image" or "im" or something else, we'll need to update all the PImage reference page examples to match. (That's why this doesn't affect only the PImage.set() page.)

prisonerjohn commented 10 years ago

Comment by alignedleft Sunday Jun 02, 2013 at 22:10 GMT


Postponing until after 2.0, as we want to do this right, but doing so will involve adjusting both the Java source and lots of PImage examples in several places. (I don't want to rush through it.)

prisonerjohn commented 10 years ago

Comment by REAS Friday Jun 07, 2013 at 20:09 GMT


This is fixed locally, will go online with next update. We'll move through the image examples and clean them up.