Change Overview from table of buttons to an IconView

[rschroll]

As discussed in issue #2, this makes the overview scroll automatically to the selected items. This changes how the overview slides are sized, though, because we can't layout the buttons and then see how big they are. I've implemented an algorithm that should result in the largest size that fits all of the slides without scrolling. (Unless this is smaller than min_overview_width, in which case that is used.)

Then, I went and got fancy, making the current slide be brighter than all the rest, instead of adding a yellow background behind it. This also lets us crowd the icons a bit closer together, and therefore have them be bigger.

The last commit enables the left and right arrows to wrap around to the previous/next row. The is different from the old behavior. I find it more natural, since it means you can always navigate with the arrow keys, but YMMV.

[davvil]

Wow! Thanks for the code. Keyboard navigation now really works well. On the other hand, now you cannot select the slides with the mouse. Probably this is easy to fix, but I didn't look into code in detail yet. And I really like the highlighting of the slides :)

Some quick comments:

• If the scrollbar is shown, there is a (relatively) big gap between it and the slides. Perhaps the sizing algorithm can be refined or else the slides could be centered. Should be visually more pleasing.
• Would it be possible to show again the slide numbers for non-rendered slides?
• If the size of the icons is known beforehand, perhaps we could combine the prerenderring with the rendering of the icons and start showing miniatures in the overview on the fly.

I will also have a look myself into these points in the next days, but feel free to tell me what you think about it.

[rschroll]

On 05/21/2012 10:22 AM, David Vilar wrote:

On the other hand, now you cannot select the slides with the mouse. Probably this is easy to fix, but I didn't look into code in detail yet.

Whoops - forgot all about that. It's a matter of 15 minutes to fix it, I think.

And I really like the highlighting of the slides :)

I'll admit it: I stole the idea from the now-defunct KeyJNote :).

Some quick comments:

• If the scrollbar is shown, there is a (relatively) big gap between it and the slides. Perhaps the sizing algorithm can be refined or else the slides could be centered. Should be visually more pleasing.

The centering of the overview slides is rather ugly right now. As far as I can tell, there's no way to tell the IconView to center the icons, and there's no way to tell it to take up as much space as it can, but not put in extra gaps. So I resorted to explicitly setting the size of the ScrolledWindow so that the IconView has the correct width. But when there's a scroll bar as well, we need to know the width of it to compensate, and I don't think we can get that until after we put enough icons in to make it appear. (Although, maybe it's stored in some settings somewhere that we can read....)

As I've been writing, I've started wondering if we can set the size request on the IconView instead of the ScrolledWindow. I'll let you know.

• Would it be possible to show again the slide numbers for non-rendered slides?

There's (at least) two ways to do this. (1) We could make a custom pixbuf for each with the number on it during fill_structure. But I worried about speed here. (2) We could adjust the custom CellRenderer to draw a number on certain slides. But this means lugging more information around in our ListStore. I think before we do this, we should look at the next question.

• If the size of the icons is known beforehand, perhaps we could combine the prerenderring with the rendering of the icons and start showing miniatures in the overview on the fly.

In the interest of not breaking things, I did the slide rendering in the same place as the previous code did. But now that you mention it, there's no reason not to do it as soon as possible. We don't want to tie it in too closely to the prerendering, since we also need to regenerate the miniatures when the overview layout changes [1]. But there's no reason not to start generating the icons as soon as prerendering is done or the number of pages changes. I'll take a look at this, but you probably have a better understanding of the event paths in question.

In relation to above, if we get the miniatures rendered quickly enough, it might not be worth the trouble to add slide numbers to the un-rendered ones. That's my hope, anyway.

[1] Actually, we only need to do this when the layout changes AND the target_width changes. Many times, changing some overviews won't trigger the second part, so there's some additional optimization possible. I don't know if it's worth the added complication or not.

[davvil]

On Mon, May 21, 2012 at 6:16 PM, rschroll reply@reply.github.com wrote:

And I really like the highlighting of the slides :)

I'll admit it: I stole the idea from the now-defunct KeyJNote :).

Well, I also took the idea of the overview from them ;)

BTW. Now it is called impressive (not the best name to look for it in google), but development also seems to have stopped there.

As I've been writing, I've started wondering if we can set the size request on the IconView instead of the ScrolledWindow.  I'll let you know.

I will wait what you find out.

[...] But there's no reason not to start generating the icons as soon as prerendering is done or the number of pages changes.  I'll take a look at this, but you probably have a better understanding of the event paths in question.

The reason I delayed the rendering before is that I didn't know the size of the buttons until the overview was shown only once. But if we now already know the size of the icons, we can start before. I will take care of this.

In relation to above, if we get the miniatures rendered quickly enough, it might not be worth the trouble to add slide numbers to the un-rendered ones.  That's my hope, anyway.

If the overview is activated early enough there will be a delay, but for most use cases the rendering will already have taken place in the background. So I don't think it is worth expending effort into this.

[1] Actually, we only need to do this when the layout changes AND the target_width changes.  Many times, changing some overviews won't trigger the second part, so there's some additional optimization possible.  I don't know if it's worth the added complication or not.

I agree. I also thought about this, but did not find it to be a priority.

[rschroll]

On 05/21/2012 04:26 PM, David Vilar wrote:

As I've been writing, I've started wondering if we can set the size request on the IconView instead of the ScrolledWindow. I'll let you know.

I will wait what you find out.

It's not looking so good. It looks like the space for the scrollbar is taken out of the child widget, not requisitioned in addition. Even if this did work, we'd end up with a scrollbar floating a bit off the edge, which looks silly (at least to me).

One possibility would be to hide the real scrollbar and draw a fake scrollbar over the right edge of the window. If we're willing to give up being able to interact with it with the mouse, it might not even be that hard to do. (But if we're going to start drawing our own widgets, we could drop the IconView and custom draw the whole overview in a DrawingArea or a clutter whatever.)

Anyway, I'm going to leave it as is and wait for one of us to have a flash of insight.

[...] But there's no reason not to start generating the icons as soon as prerendering is done or the number of pages changes. I'll take a look at this, but you probably have a better understanding of the event paths in question.

The reason I delayed the rendering before is that I didn't know the size of the buttons until the overview was shown only once. But if we now already know the size of the icons, we can start before. I will take care of this.

Beat you to it. But please check my solution.

One of the commits I pushed switched the Overview.current_slide to being a property, rather than an attribute with a getter and setter. I feel this is cleaner, but it's the only one in the codebase (that I know of). If you'd rather stick with getters and setters everywhere, I can back that change out.

[davvil]

Now it looks really good. However a new bug has appeared, probably because of the size requisition (commit 5578d03): For testing I have two monitors with different resolutions, the one with the highest resolution should show the presentation window. If I start pdfpc on this monitor, the presenter window (on the other monitor) is resized to the resolution of the bigger monitor. If I start on the monitor with the lower resolution all works fine.

I tried to solve it by delaying the adding of the overview to the presenter layout. This solves it for one of my test presentation but not for the other one. I am still looking into it. Funny thing is, the overview gets the correct size parameters in both cases.

I have also been looking at how you compute the size of the slides in the overview. We can probably make better use of the space when the scrollbar is shown by recomputing the width of the slides once we know how many columns are there (and perhaps for the time being assuming a fixed "safe size" for the scroll bar, say 30 pixels). Related to this: I do not quite follow why the code for deciding if the scrollbar is shown works. 'tr' and 'tc' should be the number of rows and columns when we go out of the loop? Then I find the test for 'tr > 0' a bit confusing.

One of the commits I pushed switched the Overview.current_slide to being a property, rather than an attribute with a getter and setter.  I feel this is cleaner, but it's the only one in the codebase (that I know of).  If you'd rather stick with getters and setters everywhere, I can back that change out.

To be honest, I am not familiar with this get/set syntax in Vala (I am learning Vala and GTK at the same time I write pdfpc). But it looks quite cool :). Do you have a link to an explanation of how it is used? get/set are not the best keywords to look up in google...

[rschroll]

On 05/22/2012 09:51 AM, David Vilar wrote:

Now it looks really good. However a new bug has appeared, probably because of the size requisition (commit 5578d03): For testing I have two monitors with different resolutions, the one with the highest resolution should show the presentation window. If I start pdfpc on this monitor, the presenter window (on the other monitor) is resized to the resolution of the bigger monitor. If I start on the monitor with the lower resolution all works fine.

I'm not sure what's causing this. I don't have an external monitor handy for testing, so I've been crossing my fingers that everything would work on multi-monitor setups.

However, I realized that we can avoid the size_request business by explicitly setting the IconView.column property. (See recent push.) I think this new method will be safer since it lets GTK work out all the sizing issues. Maybe it'll even solve your problem.

As a bonus, this makes it possible to center the overview even when the scrollbar is showing. I had to make a guess about the width of the scrollbar, but if I'm wrong, it only matters if n*target_width is just about the available space. This ends up giving a scrollbar that can float in from the right edge of the screen. It's not ideal, but it doesn't bother me that much. If you want to fix that, we could: 1) Adjust the icon width to make them fit snuggly. But then the scrollbar width becomes important. 2) Adjust the column spacing to make them fit snuggly. Again, the scrollbar width matters. 3) Figure out how to fit the Adjustment inside the ScrolledWindow, instead of the other way around. I've done this, but I haven't figured out how to hook up the adjustments so that the IconView can make the ScrolledWindow scroll when the selection changes. (Which was the whole reason we started on this path....)

I have also been looking at how you compute the size of the slides in the overview. We can probably make better use of the space when the scrollbar is shown by recomputing the width of the slides once we know how many columns are there (and perhaps for the time being assuming a fixed "safe size" for the scroll bar, say 30 pixels).

This would be option (1), above. The other problem with it, besides the scrollbar width mattering, is that it also makes the icons taller, giving you fewer visible on the screen. This isn't so desired when we're already short on space.

Related to this: I do not quite follow why the code for deciding if the scrollbar is shown works. 'tr' and 'tc' should be the number of rows and columns when we go out of the loop? Then I find the test for 'tr> 0' a bit confusing.

It worked because the while loop is bailed out of the first time through, before tr and tc are set, if there's no solution that fits the slides in at their minimal size. (Note that the target_width < min_overview_width test is a bit misleading; we're guaranteed to have either target_width == 0 or target_width >= min_overview_width.) Both those tests after the loop actually checked the same thing (did we find an acceptable layout?), which was sort of confusing and a bit silly.

In the latest commits, I've rearranged things to have only a single test of this condition at the end of the while loop. I've also documented how the algorithm works, since I'm sure even I won't remember in a week.

One of the commits I pushed switched the Overview.current_slide to being a property, rather than an attribute with a getter and setter. I feel this is cleaner, but it's the only one in the codebase (that I know of). If you'd rather stick with getters and setters everywhere, I can back that change out.

To be honest, I am not familiar with this get/set syntax in Vala (I am learning Vala and GTK at the same time I write pdfpc). But it looks quite cool :). Do you have a link to an explanation of how it is used? get/set are not the best keywords to look up in google...

I learned about them in Vala from the Vala Tutorial (https://live.gnome.org/Vala/Tutorial#Properties), though I was familiar with them from Python. The basic idea is that a property looks like an attribute from the outside, but getters and setters are called whenever its value is queried or set. Their real benefit is you can start prototyping code with attributes, and change to properties if and when you need that power, without changing the API. (There's little reason to switch from getters/setters to properties, except that I think they're nifty.) The Google keyword will be "property" (although that's not much better than "get" or "set").

[davvil]

On Tue, May 22, 2012 at 10:09 PM, rschroll reply@reply.github.com wrote:

I'm not sure what's causing this.  I don't have an external monitor handy for testing, so I've been crossing my fingers that everything would work on multi-monitor setups.

I've been doing some more tests and the bug is also in the master branch, without the IconView. Apparently it only appears with some relation of slide size - monitor resolution, and for whatever reason the IconView also triggered it on one of the two computers where I can try it. So, no need to worry too much about it in this branch.

As a bonus, this makes it possible to center the overview even when the scrollbar is showing.  I had to make a guess about the width of the scrollbar, but if I'm wrong, it only matters if n*target_width is just about the available space.  This ends up giving a scrollbar that can float in from the right edge of the screen.  It's not ideal, but it doesn't bother me that much.  If you want to fix that, we could: 1) Adjust the icon width to make them fit snuggly.  But then the scrollbar width becomes important. 2) Adjust the column spacing to make them fit snuggly.  Again, the scrollbar width matters. 3) Figure out how to fit the Adjustment inside the ScrolledWindow, instead of the other way around.  I've done this, but I haven't figured out how to hook up the adjustments so that the IconView can make the ScrolledWindow scroll when the selection changes.  (Which was the whole reason we started on this path....)

The 20 pixels you used should be a safe guess, I think. If problems arise, then we know where to look into.

[...] In the latest commits, I've rearranged things to have only a single test of this condition at the end of the while loop.  I've also documented how the algorithm works, since I'm sure even I won't remember in a week.

Now it is clearer, thanks.

From my point of view I think this is now ready for merging into master, once I solve the issue with the screen resolution (better not to include more commits than needed).

I learned about them in Vala from the Vala Tutorial (https://live.gnome.org/Vala/Tutorial#Properties), though I was familiar with them from Python.  [...]

Neat! I think I will start using them from now on (I wasn't aware of them in python either).

[davvil]

I think I finally solved the problem. For me it now works for all my test cases. You can find the changes in my overview branch. There is a relatively large number of commits as I also merged my master branch (where I thought I already had found a solution). Please check if you have problems, if not I would merge into master.

I did some small changes to the overview:

• The centering is now done in presenter, not in the overview itself (I find it cleaner and more consistent with other widgets)
• The policy for the scrollbar is set to automatic, I didn't find issues with it.

In the process I also modified code from you previous pull request about the relative sizes of the current and next views. For computing the size of the next view I take the size requested by the user for the current view, instead of the actual allocated size. Rationale is, if someone specifies a current size of, say, 90%, she expects the next view to be small. As 90% cannot be allocated for the current view, the next view ends up being much bigger of what the user was expecting.

[rschroll]

On 05/24/2012 09:17 AM, David Vilar wrote:

I did some small changes to the overview:

• The centering is now done in presenter, not in the overview itself (I find it cleaner and more consistent with other widgets)
• The policy for the scrollbar is set to automatic, I didn't find issues with it.

This does change the behavior when the overview is width-limited. Before, the icons would be centered in the available height; now, they're at the top. I like the centered appearance better, but the current behavior is cleaner to implement.

I admit I don't see what a7c67d is trying to do. We don't need to adjust for the scrollbar width unless it exists. This already happens in line 235.

In the process I also modified code from you previous pull request about the relative sizes of the current and next views. For computing the size of the next view I take the size requested by the user for the current view, instead of the actual allocated size. Rationale is, if someone specifies a current size of, say, 90%, she expects the next view to be small. As 90% cannot be allocated for the current view, the next view ends up being much bigger of what the user was expecting.

The downside of this is that there's no longer an easy way to say, "Make the current slide as big as possible, and then use the rest of the space for the next slide", which strikes me as a reasonable request. (Though I didn't realize it, this is what happened for me with the default settings. (I have a 16x9 monitor.)) While the previous behavior may have been unexpected, I don't know that it was undesired.

I guess I would have suggested adding a second option to limit the width of the next slide. This eliminates the side effect of the current slide width on the next slide width, and it gives the user more flexibility in specifying the layout, since the two limits need not add up to 100%. But despite the two paragraphs I've written, it's not really that important. If you like the current behavior, I won't complain.

[davvil]

On Thu, May 24, 2012 at 8:46 PM, rschroll reply@reply.github.com wrote:

On 05/24/2012 09:17 AM, David Vilar wrote:

I did some small changes to the overview:

• The centering is now done in presenter, not in the overview itself (I find it cleaner and more consistent with other widgets)
• The policy for the scrollbar is set to automatic, I didn't find issues with it.

This does change the behavior when the overview is width-limited. Before, the icons would be centered in the available height; now, they're at the top.  I like the centered appearance better, but the current behavior is cleaner to implement.

I didn't pay much attention to this. I changed it so that the overview sets its height explicitely, and it is also vertically centered in the Alignment in the presenter window.

I admit I don't see what a7c67d is trying to do.  We don't need to adjust for the scrollbar width unless it exists.  This already happens in line 235.

That was a solution (pretty ad-hoc) for one presentation I use for testing. The IconView first showed a configuration of the slides with a pretty big gap at the right side together with the scrollbar (I seem to remember). Then it realized that it had space for one more column and rearranged the slides in such a way that the window was resized and part of it went offscreen. However now it seems to work without this hack.

I admit sometimes I am doing kind of blind testing adding some pixels here and there. In this commit I removed the the col_spacing and row_spacing to the eff_max_width and eff_max_height. I don't see why they should be there, and it seemed to break one of the presentations I test with. It also seems that IconView adds 2_padding and 2_spacing at each icon. Correct me if I'm wrong, but I seemed to need these adjustments to make it work.

In the process I also modified code from you previous pull request about the relative sizes of the current and next views. For computing the size of the next view I take the size requested by the user for the current view, instead of the actual allocated size. Rationale is, if someone specifies a current size of, say, 90%, she expects the next view to be small. As 90% cannot be allocated for the current view, the next view ends up being much bigger of what the user was expecting.

The downside of this is that there's no longer an easy way to say, "Make the current slide as big as possible, and then use the rest of the space for the next slide", which strikes me as a reasonable request.  (Though I didn't realize it, this is what happened for me with the default settings. (I have a 16x9 monitor.))  While the previous behavior may have been unexpected, I don't know that it was undesired.

I guess I would have suggested adding a second option to limit the width of the next slide.  This eliminates the side effect of the current slide width on the next slide width, and it gives the user more flexibility in specifying the layout, since the two limits need not add up to 100%. But despite the two paragraphs I've written, it's not really that important.  If you like the current behavior, I won't complain.

Yes, this also make sense. I actually didn't pay much attention to this parameter, I inherited it from the original pdf_presenter_console. We can add this parameter, but better in another branch, let's focus this one again on the overview (It was my "fault" to start the discussion here, I know ;-))

[rschroll]

On 05/24/2012 05:54 PM, David Vilar wrote:

I admit sometimes I am doing kind of blind testing adding some pixels here and there. In this commit I removed the the col_spacing and row_spacing to the eff_max_width and eff_max_height. I don't see why they should be there, and it seemed to break one of the presentations I test with. It also seems that IconView adds 2_padding and 2_spacing at each icon. Correct me if I'm wrong, but I seemed to need these adjustments to make it work.

As best I can figure, the spacing is as follows:

• Each icon gets padding on all four size. This is the area that lights up when it is selected, and we've set it to zero.
• Each icon gets an extra bonus pixel on all four sides. (For spacing, this acts like padding, but it doesn't light up.)
• Between every row, there's row_spacing, and between every column, there's column_spacing (in addition to the paddings, above).
• Between the top/left edges and the (padding of the) icons, there's margin.
• Between the icon padding and the right edge, there's margin + column_spacing. For the bottom edge, there's margin + row_spacing.

This means the icons should have an effective width of (width + 2_(padding+1) + spacing, and have to fit into a space with effective width of (total_width - 2_margin). Previous to checking this right now, I had assumed that the right and bottom both only had margin, which is why I added column_spacing back to the effective width. I've pushed a fix to this to my branch (which doesn't have your fixes yet). Maybe you can see if that fixes your problematic example or not.

But I'm a bit worried that the details of the spacing aren't set in stone anywhere, and could vary with GTK version, theme, phase of the moon, etc. So it seems to me the less we rely on explicitly setting sizes, the better. But we obviously can't escape it entirely.

[davvil]

We ended with the same computation of the effective size at the end. I am merging into master, I think it is now mature enough.

[rschroll]

On 05/25/2012 12:32 PM, David Vilar wrote:

We ended with the same computation of the effective size at the end. I am merging into master, I think it is now mature enough.

Thanks for all the help getting this in. I have one or two niggling details, but I'll put them in separate bugs.