search

cbarbu / R-package-zoom

An R package to zoom in and out and more generally navigate a plot or a group of plots in R.
GNU Lesser General Public License v3.0
5 stars 1 forks source link

add/fix windows() support #1

Closed sgibb closed 11 years ago

sgibb commented 11 years ago

Dear Corentin,

I test your package on Windows (I don't use Windows but most of my users do) and recognize that it wasn't working in RStudio (which most Windows users use). RStudio has a graphic devices that isn't supported by setGraphicsEventHandler. That's why we need to open an extra windows() device. I replaced your XlibReplot function with replot that will handle windows/unix correctly (I hope so). I choose to use do.call("X11")/do.call("windows") to circumvent the following NOTE on CRAN:

Found an obsolete/platform-specific call in the following function: ‘XlibReplot’ Found the platform-specific device: ‘X11’ dev.new() is the preferred way to open a new device, in the unlikely

Also I replaced your class(...) == "try-error" calls by isError.

Sorry for my different coding style. Initially I wanted to use your coding style but I could find out what your style was :stuck_out_tongue_winking_eye:

Best wishes,

Sebastian

cbarbu commented 11 years ago

Hi, Thanks for all the work. I'll merge and test it make a new windows release on the git repository. I'll give it some time before submitting again to CRAN as I suspect new things with Mac will also come up.

Coretnin

On Mon, Sep 2, 2013 at 3:04 PM, Sebastian Gibb notifications@github.comwrote:

Dear Corentin,

I test your package on Windows (I don't use Windows but most of my users do) and recognize that it wasn't working in RStudio (which most Windows users use). RStudio has a graphic devices that isn't supported by setGraphicsEventHandler. That's why we need to open an extra windows()device. I replaced your XlibReplot function with replot that will handle windows/unix correctly (I hope so). I choose to use do.call("X11")/do.call("windows") to circumvent the following NOTE on CRAN:

Found an obsolete/platform-specific call in the following function: ‘XlibReplot’ Found the platform-specific device: ‘X11’ dev.new() is the preferred way to open a new device, in the unlikely

Also I replaced your class(...) == "try-error" calls by isError.

Sorry for my different coding style. Initially I wanted to use your coding style but I could find out what your style was [image: :stuck_out_tongue_winking_eye:]

Best wishes,

Sebastian

You can merge this Pull Request by running

git pull https://github.com/sgibb/R-package-zoom windows

Or view, comment on, or merge it at:

https://github.com/cbarbu/R-package-zoom/pull/1 Commit Summary

  • remove space on line ending
  • add Windows/RStudio support
  • use Authors@R and add myself as contributor
  • add NEWS file
  • replace class(...) == "try-error" by isError function

File Changes

  • M zoom/DESCRIPTIONhttps://github.com/cbarbu/R-package-zoom/pull/1/files#diff-0(13)
  • A zoom/NEWShttps://github.com/cbarbu/R-package-zoom/pull/1/files#diff-1(9)
  • A zoom/R/utils.Rhttps://github.com/cbarbu/R-package-zoom/pull/1/files#diff-2(5)
  • M zoom/R/zoom.Rhttps://github.com/cbarbu/R-package-zoom/pull/1/files#diff-3(204)
  • M zoom/man/zm.Rdhttps://github.com/cbarbu/R-package-zoom/pull/1/files#diff-4(2)

Patch Links:

Corentin Barbu-Covantes (484) 843-1580 http://scholar.google.com/citations?hl=en&user=sxDMRdQAAAAJ "For what does it profit a man to gain the whole world, and forfeit his soul?" Mark 8:36

sgibb commented 11 years ago

Hello Corentin,

I want to add keyboard support before the next release, too.

Please note that the CRAN-maintainers are not happy when you release to CRAN very often: "Submitting updates should be done responsibly and with respect for the volunteers' time. Once a package is established (which may take several rounds), “no more than every 1–2 months” seems appropriate." (http://cran.r-project.org/web/packages/policies.html)

Best wishes,

Sebastian

On Tuesday 03 September 2013 17:50:47 cbarbu wrote:

Hi, Thanks for all the work. I'll merge and test it make a new windows release on the git repository. I'll give it some time before submitting again to CRAN as I suspect new things with Mac will also come up.

Coretnin

On Mon, Sep 2, 2013 at 3:04 PM, Sebastian Gibb notifications@github.comwrote:

Dear Corentin,

I test your package on Windows (I don't use Windows but most of my users do) and recognize that it wasn't working in RStudio (which most Windows users use). RStudio has a graphic devices that isn't supported by setGraphicsEventHandler. That's why we need to open an extra windows()device. I replaced your XlibReplot function with replot that will handle windows/unix correctly (I hope so). I choose to use do.call("X11")/do.call("windows") to circumvent the following NOTE on CRAN:

Found an obsolete/platform-specific call in the following function: ‘XlibReplot’ Found the platform-specific device: ‘X11’ dev.new() is the preferred way to open a new device, in the unlikely

Also I replaced your class(...) == "try-error" calls by isError.

Sorry for my different coding style. Initially I wanted to use your coding

style but I could find out what your style was [image: :stuck_out_tongue_winking_eye:]

Best wishes,

Sebastian

You can merge this Pull Request by running

git pull https://github.com/sgibb/R-package-zoom windows

Or view, comment on, or merge it at: https://github.com/cbarbu/R-package-zoom/pull/1

Commit Summary

  • remove space on line ending
  • add Windows/RStudio support
  • use Authors@R and add myself as contributor
  • add NEWS file
  • replace class(...) == "try-error" by isError function

File Changes

Patch Links:

cbarbu commented 11 years ago

Yes I noticed that too :-)

On Tue, Sep 3, 2013 at 11:56 AM, Sebastian Gibb notifications@github.comwrote:

Hello Corentin,

I want to add keyboard support before the next release, too.

Please note that the CRAN-maintainers are not happy when you release to CRAN very often: "Submitting updates should be done responsibly and with respect for the volunteers' time. Once a package is established (which may take several rounds), “no more than every 1–2 months” seems appropriate." (http://cran.r-project.org/web/packages/policies.html)

Best wishes,

Sebastian

On Tuesday 03 September 2013 17:50:47 cbarbu wrote:

Hi, Thanks for all the work. I'll merge and test it make a new windows release on the git repository. I'll give it some time before submitting again to CRAN as I suspect new things with Mac will also come up.

Coretnin

On Mon, Sep 2, 2013 at 3:04 PM, Sebastian Gibb notifications@github.comwrote:

Dear Corentin,

I test your package on Windows (I don't use Windows but most of my users do) and recognize that it wasn't working in RStudio (which most Windows users use). RStudio has a graphic devices that isn't supported by setGraphicsEventHandler. That's why we need to open an extra windows()device. I replaced your XlibReplot function with replot that will handle windows/unix correctly (I hope so). I choose to use do.call("X11")/do.call("windows") to circumvent the following NOTE on CRAN:

Found an obsolete/platform-specific call in the following function: ‘XlibReplot’ Found the platform-specific device: ‘X11’ dev.new() is the preferred way to open a new device, in the unlikely

Also I replaced your class(...) == "try-error" calls by isError.

Sorry for my different coding style. Initially I wanted to use your coding

style but I could find out what your style was [image: :stuck_out_tongue_winking_eye:]

Best wishes,

Sebastian

You can merge this Pull Request by running

git pull https://github.com/sgibb/R-package-zoom windows

Or view, comment on, or merge it at: https://github.com/cbarbu/R-package-zoom/pull/1

Commit Summary

  • remove space on line ending
  • add Windows/RStudio support
  • use Authors@R and add myself as contributor
  • add NEWS file
  • replace class(...) == "try-error" by isError function

File Changes

Patch Links:

— Reply to this email directly or view it on GitHubhttps://github.com/cbarbu/R-package-zoom/pull/1#issuecomment-23723739 .

Corentin Barbu-Covantes (484) 843-1580 http://scholar.google.com/citations?hl=en&user=sxDMRdQAAAAJ "For what does it profit a man to gain the whole world, and forfeit his soul?" Mark 8:36

sgibb commented 11 years ago

I have seen you partly merge this commits already manually. Should I merge your new master into this pull request?

cbarbu commented 11 years ago

Sorry, I'm beginning in the pull request system, I'm not entirely sure of what you mean.

What I did: I merged on my computer your pull request, then I edited/ added things and pushed it back to github (I have done a lot of things since then).

What I understand you should do: pull everything I've done (I'll not be working on it in the next couple of hours and it's all pushed), merge as needed and make a new pull request.

Does this make sens or am I missing a key functionality of git/ github?

Also I've been looking into roxyPackage to further automate the administration of the package have you used it ever?

Corentin

On Thu, Sep 5, 2013 at 3:08 PM, Sebastian Gibb notifications@github.comwrote:

I have seen you partly merge this commits already manually. Should I merge your new master into this pull request?

— Reply to this email directly or view it on GitHubhttps://github.com/cbarbu/R-package-zoom/pull/1#issuecomment-23893488 .

Corentin Barbu-Covantes (484) 843-1580 http://scholar.google.com/citations?hl=en&user=sxDMRdQAAAAJ "For what does it profit a man to gain the whole world, and forfeit his soul?" Mark 8:36

sgibb commented 11 years ago

I merged your commits to my repository and updated the pull request. You could use the github webinterface to merge this. Have a look at: https://help.github.com/articles/merging-a-pull-request I never used the roxyPackage.