GNOME Bugzilla – Bug 672601
wizard: Don't explode the window with lots of medias
Last modified: 2016-03-31 14:00:01 UTC
Check commit log for details.
Created attachment 210301 [details] [review] wizard: Don't explode the window with lots of medias Although an unlikely scenerio for a typical end-user, its still very much possible that user has lots of bootable ISOs and/or HW media. We should limit ourselves to first few (6) medias in that case instead of presenting a broken/unusable UI.
Review of attachment 210301 [details] [review]: I think we should either have a scrollbar or a "next/more" button
(In reply to comment #2) > Review of attachment 210301 [details] [review]: > > I think we should either have a scrollbar After a bit more experiments, thinking and discussion with Company, I don't think thats very much possible without making the UI ugly: <zeenix> there is a Menubox class that derives from Box and we add round corners nicely to it <zeenix> http://static.fi/~zeenix/tmp/menubox+scrolledwindow.png <zeenix> Company: this is after puttting it under a GtkScrolledWindow <zeenix> http://static.fi/~zeenix/tmp/menubox.png <Company> you can't get the rounded box there <Company> because there's a GdkWindow involved <Company> looks like crap anyway with the scrollbar <zeenix> yeah <Company> you could make the viewport color transparent <Company> or is that a treeview? <Company> or the same dark gray as the surroundings <Company> ten you'd at least get rounded corners at the first/last button <zeenix> there is no treeview <zeenix> its a box <zeenix> GtkBox subclass <Company> so it's a viewport <Company> the direct child of the scolled window <zeenix> well, box inside a viewport <Company> right <Company> but boxes are transparent anyway <zeenix> perhaps it'd be better to use a notebook with next/prev buttons instead <Company> dunno <Company> the whole design looks very unscrollable <Company> (a) because it has rounded corners that group it <Company> (b) because it's only using half the window <zeenix> yes my first solution was to just limit the number of medias presented <Company> you could have a "more..." :) <zeenix> true > or a "next/more" button As pointed out at the end of discussion above, something like that might be more appropriate. I'll look into this possibility now..
(In reply to comment #3) > As pointed out at the end of discussion above, something like that might be > more appropriate. I'll look into this possibility now.. Did you get input from the design team on this issue?
(In reply to comment #4) > (In reply to comment #3) > > > As pointed out at the end of discussion above, something like that might be > > more appropriate. I'll look into this possibility now.. > > Did you get input from the design team on this issue? Yeah, I was going to add 'ui-design' flag and ask them but I came-up with another possibility with scrolledwindow so I'll try that first. :) If that doesn't work, this 'prev' and 'next' solution would take a while I think, so we should for now go with the workaround patch I already provided. IMO its much better to make user go through the file chooser instead of an unusable UI.
Created attachment 212178 [details] [review] Put wizard source under scrolled window
Created attachment 212179 [details] Screenshot of attachment#212178 [details] Since I found no way to add rounded corners to a GtkScrolledWindow or GtkViewport, I had to put a separator at top. Suggestion on how to do this in a cleaner/simpler way are more than welcome!
Comment on attachment 212178 [details] [review] Put wizard source under scrolled window This has a few issues, I'll provide an improved version soon.
Created attachment 212400 [details] [review] Put wizard source under scrolled window Improvements over the previous version: * Only show media menu when there is medias * Always show full media items when scrolled * Adds separators before and after media menubox * Removes hacks to show medias in alphabetical order Still two critical issues when there is no media to be shown: * An ugly big gap where scrolled window is supposed to be. This can be solved by adding/removing scroll window instead of showing/hiding it but that leads to the 2nd issue: * Top rounded-corners look like a tumor.
Created attachment 212480 [details] [review] Put wizard source under scrolled window This version has following fixes/improvements: * Make wizard source look unugly (not really very pretty but still acceptable). * Add rounded borders to URL menubox too. * Put rounded border addition into separate function. BTW, Some notes on this work: a. I've been in communication with jimmac on what is acceptable and what is not. b. Feel free to point out any issues (thats the use of presenting it) but keep in mind that I've tried various (more simpler/obvious) approaches but failed to make any of them work, mainly due to a limition of gtk: You can not make GtkScrollWindow use rounded corners. So if you think this can be done in a better/simpler way, I will be more than happy to throw away this patch in favor of yours.
Created attachment 212614 [details] [review] Put wizard source under scrolled window Yet another revision: * Remove black border around viewport * Put a separator above media scrolled window * Remove an unneeded hack to get rid of scrollbars * Some renaming
Created attachment 212618 [details] Screenshot with lots of media
Created attachment 212619 [details] some media (no scrolling)
Created attachment 212622 [details] screenshot with no media available I know the 'disclaimer' should not be shown in this case but thats a separate bug.
While I would have preferred not having to prepend and append the rounded corner part, the proposed solution is reasonable.
The implementation isn't going in a nice direction though. MenuBox was supposed to be like a regular GtkBox, only styled differently, so items in it receive a background that gets highlighted/colored when the child has the enter/focus. I don't pretend it is fantastic, but it was getting the job done, and fairly nicely. There was a clear seperation between style and usage. With this patch it's all mixed up and that's wrong. All those add_rounded_border() should not be there. num_items is probably unnecessary, it's a regular gtkcontainer, you can get_children(). I don't know what are the exact problems to add a scrolled, i can imagine there are many, and I am not more familiar than you with Gtk, but I believe there is room for improvements and we should try to stay on a correct path: making it a proper widget, at least from usage point of view and not mix gtk hacks and logic. Can you resume/explain why a scrolled window with transparent background couldn't be put as the first child? My understanding is that it would look already nicer. The scrolled window would have "no-background" childs/items (like now, iirc), and menubox could learned to be styled differently when embedded inside a scrolled window: focus/highlight on top level item (not the item containing the scrolledwindow), draw background & top border according to interesection/mask with parent menuitem.
(In reply to comment #16) > Can you resume/explain why a scrolled window with transparent background > couldn't be put as the first child? I don't think we discussed that and it didn't occur to me. :( > My understanding is that it would look > already nicer. I had the same thoughts on a few other approaches i tried but it wasn't true so I won't believe it until its tried. :) > The scrolled window would have "no-background" childs/items > (like now, iirc), and menubox could learned to be styled differently when > embedded inside a scrolled window: focus/highlight on top level item (not the > item containing the scrolledwindow), draw background & top border according to > interesection/mask with parent menuitem. I suppose I could try that but what about the scrollbar? How about we don't have rounded-corners on the top? jimmac just told me on IRC that would be acceptable.
(In reply to comment #17) > I suppose I could try that but what about the scrollbar? How about we don't > have rounded-corners on the top? jimmac just told me on IRC that would be > acceptable. sounds like a better approach to me
Created attachment 212652 [details] [review] Put wizard source under scrolled window This one is way simpler but rids of rounded-corners on the top. Its probably easy to keep the rounded corners in case of no media but that would be inconsistent I guess.
Created attachment 212654 [details] Screenshot with lots of media
Created attachment 212655 [details] screenshot with 6 medias
Created attachment 212656 [details] screenshot with no media available
As discussed on IRC, it's ok to keep the scrolled list have no rounded corners, but in cases where we don't scroll, the whole widget should have rounded corners (including the top).
Created attachment 212733 [details] [review] Add scrolling to media menu when we have >6 medias Here is a version that doesn't add redundant separators to achieve rounded corners. Now everything looks the same as before until 7th media is added. At that point, medias are put into a scrolled window. Also at this point we also lose the rounded corners on the top but as jimmac pointed out in comment#23 thats very much acceptable.
Created attachment 212734 [details] Screenshot with 7 medias
Created attachment 212735 [details] screenshot with 6 medias
Created attachment 212736 [details] screenshot with no media available
6 and 7 should not be hardcoded, the windows may have been resized to something smaller, or a smaller/bigger screen that what you test on may be used.
(In reply to comment #28) > 6 and 7 should not be hardcoded, the windows may have been resized to something > smaller, or a smaller/bigger screen that what you test on may be used. As long as the window is at least 857x733, all UI is visible/usable (I tested this) and its not a big resolution to expect. Besides when not maximized, its very easy to resize window. About the number 6, I and jimmac agreed its a good number (typical user shouldn't even have this much so this is pushing already).
(In reply to comment #29) > > As long as the window is at least 857x733, all UI is visible/usable "640kB ought to be enough for anybody"
(In reply to comment #30) > (In reply to comment #29) > > > > > As long as the window is at least 857x733, all UI is visible/usable > > "640kB ought to be enough for anybody" haha! I didn't say it should be enough but that thats the least Boxes should get. I haven't yet checked how this UI would look like on a large screen but starting to scroll after 6 items is not very relevant as you can't expect user to have enough medias around to make the media menu large enough for her/his screen.
netbooks are something x 600, and if people are not expected to have more than 6 items in the list, why are you doing all this work?
(In reply to comment #32) > netbooks are something x 600, and if people are not expected to have more than > 6 items in the list, why are you doing all this work? "Typically" but if they have more, we are supporting that usecase with this patch. Also you seem to have missed an important point in the previous comment.
(In reply to comment #33) > (In reply to comment #32) > > netbooks are something x 600, and if people are not expected to have more than > > 6 items in the list, why are you doing all this work? > > "Typically" but if they have more, we are supporting that usecase with this > patch. Also you seem to have missed an important point in the previous comment. Its not so black&white. We support the use case but shouldn't go too far (I've already spent a lot of time on this) to make UI perfect for non-typical use cases.
Really hardcoded limits are bad, this will already cause issues in some setups, this will annoy people who want to resize boxes window, this will suck when we want to reuse that code for another purpose, ...
(In reply to comment #35) > Really hardcoded limits are bad, this will already cause issues in some setups, > this will annoy people who want to resize boxes window, this will suck when we > want to reuse that code for another purpose, ... If you feel so strongly about this and don't want to address my argument in comment#31. Feel free to modify the patch to make it perfect for all resolutions and screen sizes.
argument? You mean the unilateral decision you just made because it's so convenient here? When we need to add something above or below the ISO list, or when we change the trademark sentence, will we just change the minimal window size requirement? How come I can resize the window below the minimal size?
(In reply to comment #37) > argument? You mean the unilateral decision you just made because it's so > convenient here? When we need to add something above or below the ISO list, or > when we change the trademark sentence, will we just change the minimal window > size requirement? How come I can resize the window below the minimal size? No, you keep on missing it so I'll stop arguing. Talk is cheap so I'll now expect you to provide a modified patch or forever hold your silence.
Comment on attachment 212733 [details] [review] Add scrolling to media menu when we have >6 medias No problem with being silent on this bug now. I haven't looked at the code except to check if 6/7 was really hardcoded, so this patch still needs to be reviewed.
(In reply to comment #39) > (From update of attachment 212733 [details] [review]) > No problem with being silent on this bug now. I haven't looked at the code > except to check if 6/7 was really hardcoded, so this patch still needs to be > reviewed. Any progress on this patch?
+ } else if (media_menubox.num_items == 7) { + if (media_menubox.num_items == 6) { + for (var i = 0; i < 11; i++) { // 6 items + 5 separators it surely is possible to do better than that...
Uhm, I'm looking at this, and I'm not really liking MenuBox. Its not handling clicking right (doesn't wait until release), it doesn't have much keyboard navigation, it uses junctions rather than nth-child selectors, etc. I started a list-like widget for Contacts that has a bunch of features that would make sense here, including things like optional per-row separators that would clean this up a lot. I'd like to spend some time finishing that, then we could all reuse it to much rejoicing.
(In reply to comment #42) > Uhm, I'm looking at this, and I'm not really liking MenuBox. Its not handling > clicking right (doesn't wait until release), it doesn't have much keyboard > navigation, it uses junctions rather than nth-child selectors, etc. Yup, 100loc not written by a Gtk developer, what you expect ;) (keyboard navigation is first broken by Clutter-Gtk) > I started a list-like widget for Contacts that has a bunch of features that > would make sense here, including things like optional per-row separators that > would clean this up a lot. I'd like to spend some time finishing that, then we > could all reuse it to much rejoicing. that would be awesome!
(In reply to comment #42) > Uhm, I'm looking at this, and I'm not really liking MenuBox. Its not handling > clicking right (doesn't wait until release), it doesn't have much keyboard > navigation, it uses junctions rather than nth-child selectors, etc. > > I started a list-like widget for Contacts that has a bunch of features that > would make sense here, including things like optional per-row separators that > would clean this up a lot. I'd like to spend some time finishing that, then we > could all reuse it to much rejoicing. Awesome! How soon do you think we'll have that new widget? I'm only asking this cause currently the menubox is completely unusable if user happen to have a few more ISOs then we expect him/her to have so I'd like to fix this bug first ASAP (It has already waited long enough).
I'm working on the other widget atm, hope to have something this week.
I just landed an initial version of the widget in the egg-list-box repo and started using it in Contacts. I'll do some more polish of it (like docs) and then I'll try to use it in boxes.
I landed my custom widget in the egg-list-box git module and its being used in contacts and empathy. However, it turns out that due to some widget implementation details its not really a good match for something themed like the boxes widget, so I'll have a look at just making the existing boxes code nicer instead.
Created attachment 214739 [details] [review] Use buttons instead of MenuBox There is no need for custom widgetry, we just use regular buttons with the right theming. Also, there is no need for separate separators, we can do that with the themeing too.
Created attachment 214740 [details] [review] Make it possible to scroll the media box
Created attachment 214741 [details] [review] Remove unused MenuBox
Created attachment 214753 [details] screenshot, before alex
Created attachment 214754 [details] screenshot, after alex changes
Created attachment 214755 [details] square corner
overall looks good to me, It seems that when the width is limited, the media items don't get rounded corners (see attached the square corner screenshot). I guess it should be fixable since the non-media items always do. The separators are a not "flat" anymore, is that intentional? Also, selected item don't get blue background anymore, but a surrounding square line (button style). I think we wanted total visible items to be 6 (media_scrolled = new WizardScrolled (4)). No idea what number is best. nitpick, the indentation (though I don't mind much) ex: button.clicked.connect ( () => { clicked (); }); for consistency should be: button.clicked.connect (() => { clicked (); }); As you said on irc, a few leftover from removing some css.
The separator look are intentionally changed to match the mockups. I'm not sure about the exact behaviour we want for the blue. The mockups show the first item in blue, but with current boxes i only get blue at seemingly random times. When exactly is an item "selected"? Is it when it has focus? Or when active (mouse button pressed down)? Oops on the 4, that was a debug leftover. I think the square thing is a problem with width of the rows, if we make the labels ellipsize or wrap I think it'll go away.
As discussed on IRC, let's use the blue for :active and a lighter shade of grey for :hover and :focus.
Created attachment 214767 [details] [review] Remove unused CSS rules
Created attachment 214768 [details] [review] Make sure to ellipsize menu labels Without this the rounded corners will not end up in the right place
Created attachment 214769 [details] [review] Fix background colors in menus We use blue for active and the brighter grey for both the prelight and the focus state. Additionally we drop the focus rect since we're using color for the focus.
Created attachment 214770 [details] [review] Fix up indentation
ack from me, once squashed the last patches
squashed and pushed