[rkward-devel] paginate onscreen graphics device

[Prasenjit Kapat]

Hi,

On Mon, Jun 28, 2010 at 11:01 AM, Thomas Friedrichsmeier
<thomas.friedrichsmeier at ruhr-uni-bochum.de> wrote:
> Hi,
>
> On Monday 21 June 2010, Prasenjit Kapat wrote:
>> Basically, something along the following lines:
>>
>> RKGlobals::rInterface ()->issueCommand ("rk.record.plot$onDelDevice ("
>> + QString::number (device_number) + ')', RCommand::App, i18n ("Add
>> current plot before closing device number %1", device_number),
>> error_dialog);
>
> what I have done now, is to simply call dev.off() when the user tries to close
> the device using Window->Close or the close-button. That way, we have only a
> single place where onDelDevice() is called from, which is always a good idea.

I haven't tested the new code yet, but is there a 'catch-22' issue
here? The dev.off () bug was solved by forcefully closing / killing
the window and now the closing itself is mapped to dev.off ()?

>> 2a. A better solution would be provide a toolbar for the graphics
>> window. Then have them under View and add them to the toolbar as well.
>> The toolbar, in far enough future, could be used to add more "editing"
>> features!
>
> As you can see, I've added a toolbar, although I guess the "First plot", "Last
> plot", and "Clear History" actions should not go into the toolbar by default,
> in the final version. Suggestions on better icons are always welcome, at this
> and all other places.

Yes, I saw the toolbar. It is way more appealing now. I am fine with
the current icons. These are cosmetic issues.

>> Opinions, suggestions, criticisms are welcomed.
>
> A few more things:

Always helpful. I have few other things in the process. I'll take it
up on a separate thread later.

> - On line 75 of public_graphics.R, I think it should be
>                current [[deviceId]] <<- length (recorded)+1L
> (if the previous device *was* the null device, the new window should still be
> at the end of the current history). I don't really understand the
> newPlotExists-list. No idea, whether or not the same problem applies, there.

If you mean this line:

current [[deviceId]] <<- current [[duplicateId]]

then, the 'duplicateId' is needed because: when Device > Duplicate is
called, I want to keep the the new window at the same history position
as the old window. Moreover, the length increment happens only when a
valid record () call is made; not necessarily when a device is added.
Or may be I am missing your point.

newPlotExists just tracks whether or not there is any _new_ plot on a
device which hasn't yet been added to the history.

It relates to the fact that when record () is called from plot.new ()
the _previous_ (which is now assumed to be "complete") plot gets
recorded and not the current (possibly incomplete) one.

> - In dev.off(): I believe my suggestion to use dev.interactive() was not a good
> idea, after all. dev.off() allows to specify a device number which is not
> currently active, but dev.interactive only returns info on the active device.
> Alternative suggestion: If a known current index exists, then obviously this
> device was "managed" in the history. If the index is NA/NULL, then it was
> probably unrelated. I.e. always call onDelDevice from dev.off(), but make
> onDelDevice smarter.

I was delineating between two concepts: (1) rk.record.plot () and
anything inside it deals only with storing/deleting/replaying plots
and in that sense is called only for interactive non-preview devices
and (2) the onus of calling record / onAddDevice / onDelDevice rests
on the calling function.

But now I think, this is not a wise way to delineate stuff. Going by
your way, I will want to make onAddDevice () and record () smarter as
well; taking the respective burdens off rk.screen.device () and
plot.new (). Just to be consistent, if possible. I'll dig into this.

> - Perhaps some code could be made easier by casting the device numbers to
> string using as.character(). Then the device numbers could be used as "real"
> names in the list, instead of indexing. But this concerns aesthetics, only,
> and may not be worth the trouble.

This is a much better idea. It should take care of the situation when
device n+1 is an open-interactive-non-preview device, but device n is
either non-existent on a non-interactive or a preview device.

> - Once the details of your code are sufficiently stable, it would be good, if
> you could add one or more tests to rkward_application_tests.R. Naturally, not
> all things can be tested from there, but you could add some checks, whether
> all indices are correct after adding/removing devices, etc.

Let me bring the code to a more "stable" and acceptable state first!

-- 
Prasenjit