ejeschke / ginga

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

Thumbs and/or Contents to highlight currently displayed image #186

Closed pllim closed 8 years ago

pllim commented 9 years ago

As pointed out by @hcferguson, currently Thumbs and Contents (at least in Qt) currently do not highlight their thumbnail/entry that is being currently displayed. It would be convenient to have the active image highlighted so user knows its corresponding thumbnail and Contents entry without having to manually cross-reference via "Synopsis" (under Info) first.

pllim commented 9 years ago

Thumbs

Maybe we can update focus_cb() to change thumbnail label color, say, to red, for the active image. However, I cannot find a trivial way to do this for Qt (since I don't really care about GTK) because focus_cb() is in ThumbsBase that is supposed to be toolkit-indepedent.

One idea is to add a method, say, highlight_thumb(), to do the actual highlighting. Call it from Thumbsbase.focus_cb(), implement it in qtw/plugins/Thumbs.py, and just have the GTK version to pass without actually doing anything.

@ejeschke, what do you think?

See #195 for Qt solution.

Contents

See #190 for Qt solution.

ejeschke commented 9 years ago

This is actually a more tricky problem than it may first appear. One complicating factor is that with alternative layouts you can have several channel windows visible--then you need to keep track of, and highlight, all the thumbs and contents entries for all the images in visible windows (and it is very easy to make an alternative layout--you can do it by just specifying a new layout in your .ginga/ginga_config.py). Because plugins have a lot of latitude over their channel windows as well, they can manipulate them to load new images (think MultiDim, or Mosaic) so this also complicates the viewer trying to keep track of whether an image is still "being viewed" somewhere.

We could experiment with this by adding a callback that would be the counterpart to the image-set callback (i.e. image-unset?). This would be called when an image is displaced in the viewer by another one. Either that or change the method signature of image-set to also include the image that was being displaced.

gtk version definitely needs to follow what the qt version is doing, as much as possible, in the general scheme of things for the reference viewer. But it's pretty straightforward to change the background of a label in both desktop widget sets.

pllim commented 9 years ago

I don't think STScI has a need for GTK. If I submit the solution for Qt, is it possible for someone else to implement GTK later on?

ejeschke commented 9 years ago

Well, preferably if the solution can be generalized (e.g. "change the background of a label") than I can implement the corresponding behavior in gtk so that it comes out at or around the same time. But in regard to the points mentioned above, they apply even to the Qt version, so if you have a user that makes an alternative layout or plugin, your highlight scheme might show the wrong things, if you don't take that into account. If you are "locking down" or assuming specific use patterns in the "stginga" version then you might be able to implement a simple solution--in which case maybe the way to go is to just make a custom version of Thumbs and distribute that (don't you have a custom Contents?)

My issue with only improving one of the platforms is that if the supported functionality is too disparate between the widget sets then the documentation appears to be incorrect for one or more of them, and the issue tracker will fill up with "bugs" reported for the non-implemented platform. That is why I am trying to merge all of the plugins into "wrapped" versions, because then the enhancements roll out automatically to all supported widget sets. Same thing for OS: I really want to make sure that I don't just assume everyone is running Mac OS X or Linux and only fix things that affect those platforms.

I've been around long enough to see a few good programs get abandoned because they are too closely tied to a widget set. By keeping the core technology abstracted from the widget set ginga can keep evolving with new platform developments, like a web version, etc. This is a design parameter for ginga and by keeping it not too Qt-centric it makes sure that there are always alternative options open, and good migration options for unforseen future use scenarios.

ejeschke commented 9 years ago

@pllim, commit a9bcc1b2fadd624282f10a3c8e4d6e7c62a2cb28 contains the "image-unset" callback--it may be useful if you are interested in pursuing this. When a new channel is created (see add_channel() in ThumbsBase) you would want to register for this and the "image-set" events, they will tell you when an image is replaced in a channel using the "typical" ways. It's probably the best way of trying to keep track what is going on with the displayed images.

pllim commented 9 years ago

@ejeschke

gtk version definitely needs to follow what the qt version is doing

Yes, I understand. Why don't we agree on the Qt implementation first, and then we can talk about the equivalent form for GTK? Currently, my idea is to just highlight the font color in red, so shouldn't be too complicated.

We could experiment with this by adding a callback that would be the counterpart to the image-set callback (i.e. image-unset?).

I am using the active-image callback, which seems to work pretty well in Qt. Given #180, I cannot really test if that works for detached workspace/channel, since it is currently not possible in Qt.

ejeschke commented 9 years ago

Sounds good. I left some comments in the PR.

pllim commented 9 years ago

Thanks, I'll take a look next week. Happy Halloween! :ghost: :jack_o_lantern:

pllim commented 9 years ago

@ejeschke , I updated the two relevant PRs. Please let me know what you think.

ejeschke commented 9 years ago

@pllim, can you please try commit 915dad34bbadcab710fac3b7f2b1bc15101850f4? This adds support for the highlight function in the Contents. It works a little differently then your PR, it tracks the "image-set" callback, which allows ginga to track the visible images in all channels, not just following the focus.

Let me know what you think.

ejeschke commented 9 years ago

We can add back in the stuff about making the color a preference, etc.

pllim commented 9 years ago

@ejeschke, your commit seems to work (for channels anyway, since I can't figure out how to open images in a workspace anymore without drag-and-drop); see attached screenshot. However, I don't see alternate row colors anymore in Contents -- Is this intentional? Also, the behavior is not exactly the same as what I implemented for Thumbs in #195. Personally, it is clearer to me to just highlight one at a time (the one actually shown), but I cannot speak for the targeted users.

untitled
ejeschke commented 9 years ago

I reverted the alternate color scheme, but I am open to bringing it back. Actually, since it is a kwarg to the widget creation we could make it a plugin preference. Do you prefer it?

ejeschke commented 9 years ago

The behavior is at the crux of the issue. In opening this issue, you say:

As pointed out by @hcferguson, currently Thumbs and Contents (at least in Qt) currently do not highlight their thumbnail/entry that is being currently displayed. It would be convenient to have the active image highlighted so user knows its corresponding thumbnail and Contents entry without having to manually cross-reference via "Synopsis" (under Info) first.

The behavior of Contents is now consistent with that goal--it highlights the images that are being displayed in the various widgets. However, if the goal is to just highlight the image for the widget that currently has the keyboard focus, then we would want to track the callback you are using in your PR.

ejeschke commented 9 years ago

BTW, you can use FBrowser or "File"->"Load Image" with channels displayed in a grid. Simply click on a window in the grid to select that channel, and then invoke the FBrowser plugin (or File->Load Image).

pllim commented 9 years ago

@hcferguson , which highlighting scheme do you prefer?

pllim commented 9 years ago

Ah yes, I am so fixated on the drag and drop problem that I forgot I could manually load the files as well.

@ejeschke, when I click on a different channel in the workspace and load an image using FBrowser, it always loads the image to the first channel in the workspace regardless. However, using File -> Load loads it to the correct workspace channel.

ejeschke commented 9 years ago

Do you have channel_follows_focus = True in your general.cfg?

pllim commented 9 years ago

No, I don't have it set. I am using the software default.

ejeschke commented 9 years ago

I just pushed a commit that adds a preference to Contents called "highlight_tracks_keyboard_focus". You can play with the setting both ways.

ejeschke commented 9 years ago

No, I don't have it set. I am using the software default.

Does the Pan display change as you move the mouse over the other grid windows, or only when you click on them?

pllim commented 9 years ago

Is the Pan display the small display thingy under Info? If so, it only updates when I click on a channel in the workspace.

ejeschke commented 9 years ago

I see. I think you are reusing the same FBrowser instance over and over. But since it is a local plugin it is tied to the channel that you started it from. If you open a new FBrowser after clicking a different image you will be loading files into that new channel.

ejeschke commented 9 years ago

Dragging the files from the FBrowser is different because you are dragging them to a specific channel.

ejeschke commented 9 years ago

We will bring back dragging to FBrowser so don't worry about it.

ejeschke commented 9 years ago

Dragging files from FBrowser restored in commit 0c39d3d933b7bf412e6818cf9e28ec395d98d7f6.

pllim commented 9 years ago

@ejeschke , thanks!

The alternate color scheme makes reading a long list easier, but it is not that critical if it is gone. I see that FBrowser has it. So, why is it added for one plugin but removed for another?

The new setting to only highlight focused image mostly works, but not quite there yet. I'll see if I can submit a fix as part of my existing PR when I get a chance. Included in the PR.

ejeschke commented 9 years ago

I think the only plugin that currently is somewhat incompatible with the alternating color scheme is Catalogs, since it selects rows when you highlight a star in the field (you can select stars from either the table or the image). And when a row is selected it is difficult to see that it is in fact selected amidst the alternating color pattern.

ejeschke commented 9 years ago

Commit 6eaf58fbd467ec02d53d10c2bb3a0846e81c9ce7 makes alternating row color scheme a preference for plugins Contents, FBrowser and Header. Default is True.

ejeschke commented 8 years ago

I believe this is resolved with recent PR merges of #190 and #195. Still not sure the default scheme makes sense when multiple workspaces are shown, or a grid workspace, but maybe that is a future issue.

pllim commented 8 years ago

Thanks, @ejeschke ! By "default scheme", are you talking about highlight_tracks_keyboard_focus in plugin_Contents.cfg and plugin_Thumbs.cfg? I agree with opening a new issue if we need to revisit the current default values.