search

ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Remove the combination Viewer and Canvas #973

Closed ejeschke closed 2 years ago

ejeschke commented 2 years ago

The PR removes the combination Viewer-Canvas. As a consequence, you cannot draw on a viewer, only a canvas and you cannot zoom a canvas, only a viewer.

This change has been phased in for a long time so that there shouldn't be too much legacy code that uses it. But it is not backward compatible as the combination classes have been eliminated. Some of the older examples were still using this feature. Users using the old classes will need to update their code. For the most part, this is a rather straightforward change from ImageViewCanvas to CanvasView.

ejeschke commented 2 years ago

Rebased against #991

ejeschke commented 2 years ago

@pllim, I'd like to merge this as part of the 4.0 release. I found the only thing affected in stginga is in an experimental plugin. You can probably just change this from ImageViewCanvas to CanvasView and it should still work.

pllim commented 2 years ago

Seems pretty drastic. Is there no way to support also the old way of things for a while during the deprecation period with a warning?

ejeschke commented 2 years ago

Normally, I would agree. BUT--I think the time to make this kind of change is on a major release (i.e. 3.x.y => 4.0.0). Since almost all the examples were transitioned away from the dual object a few releases ago, and most all plugins are not affected by this, I think it is a good time to do it.

I'd rather make a few API breaking changes on the transition to 4.0.0 and then (like the 3.0 series, have more gradual changes with deprecations) in the 4.x series.

ejeschke commented 2 years ago

Did you try stginga plugins with this PR?

pllim commented 2 years ago

Not yet. Do you have a deadline I should shoot for?

ejeschke commented 2 years ago

Any chance you could check those in the next week or so? I tested stginga with general usage with this PR, but I'm not so familiar with the STScI plugins. I can submit a PR for the MultiImage plugin, but I might need help testing it. If you think that one is not likely to be used anytime soon then we don't have to worry too much about it.

pllim commented 2 years ago

MultiImage is likely very broken by now, so I can try patch it theoretically but there is no way to really test it. I will put this PR on my queue. Thanks for your continue patience!

ejeschke commented 2 years ago

See my MultiImage PR for the fix (one-liner), plus fixed up a bunch of other calls that are likely to be deprecated in the future.

pllim commented 2 years ago

Looking at this today. FYI.

ejeschke commented 2 years ago

But I think the risk of incompatibility is low here.

Most code modeled in Ginga uses canvases and viewers independently. If there is anything needing migration it would be use of the ImageViewCanvas class--but you can just substitute CanvasView.

ejeschke commented 2 years ago

Thanks for the review, @pllim!