GNOME Bugzilla – Bug 590493
Rewrite Dash, remove hardcoded width/height from GenericDisplay
Last modified: 2009-08-05 08:06:25 UTC
This patch is a near-total rewrite of the Dash. First, the dash code moves into a separate file, dash.js. Inside dash.js, the components are more broken up into separate classes; in particular there's now a Pane class and a MoreLink class. Instead of each section of the dash, when activated, attempting to close all N-1 other sections, instead there is the concept of a single "active pane", and when e.g. activating the More link for documents, if we know there's an active pane which happens to be the apps, close it. Many redundant containers were removed from the dash, and all manual width, height and x/y offsets are entirely gone. We move the visual apperance closer to the design by using the view-more.svg, etc. To complete the removal of height/width calculations from the dash, we also had to do the same for GenericDisplay. The main regression with this patch is that that details link is not implemented yet.
Created attachment 139706 [details] [review] Rewrite Dash, remove hardcoded width/height from GenericDisplay
Note that this patch will be an intense conflict-o-rama with almost any other work in the overlay/dash area, so it'd be good to get it in sooner rather than later. But it should make other things like say adding a more link to the Places an actually palatable concept.
*** Bug 587000 has been marked as a duplicate of this bug. ***
*** Bug 578197 has been marked as a duplicate of this bug. ***
Created attachment 139815 [details] [review] Rewrite Dash, remove hardcoded width/height from GenericDisplay Rewrite Dash, remove hardcoded width/height from GenericDisplay This patch is a near-total rewrite of the Dash. First, the dash code moves into a separate file, dash.js. Inside dash.js, the components are more broken up into separate classes; in particular there's now a Pane class and a MoreLink class. Instead of each section of the dash, when activated, attempting to close all N-1 other sections, instead there is the concept of a single "active pane", and when e.g. activating the More link for documents, if we know there's an active pane which happens to be the apps, close it. Many redundant containers were removed from the dash, and all manual width, height and x/y offsets are entirely gone. We move the visual apperance closer to the design by using the view-more.svg, etc. To complete the removal of height/width calculations from the dash, we also had to do the same for GenericDisplay.
It's cool to have the size allocation of the overlay components work dynamically! We should first fix the regressions though before we can commit this code. Visual/behavior problems: 1) Dash is missing the background color. 2) Did you remove the gradient for the dash and panes intentionally? I thought it was a bit nicer with it. 3) Highlights on the More and Close buttons on mouse-over look bad. Is it possible to get rid of them? There is another image called view-more-activated.svg that should be used when the More results are being shown. 4) Old selection should not be getting preserved when a More results pane is closed and then opened again. 5) Details pane is still not showing up for More results items. 6) Clicking (i) the second time should be closing the details pane for the item. 7) Search results are missing section headers. 9) Clicking More when viewing search results should narrow down the results to that type of items. Conversely, search should apply only to a particular type of items when More is already selected. 9) "More" pane for documents should not be as wide. + let docsDisplay = new ResultArea(DocDisplay.DocDisplay, true); Perhaps you need to pass in false here. 10) Drop no longer works. Possibly because you are missing lower_bottom() call for this._paneContainer Comments about the code: 1) - this._wordWidth = Shell.Global.get().get_max_word_width(this.actor, appInfo.get_name(), - "Sans 12px"); Probably need to remove get_max_word_width() function itself if we don't intend on using it. 2) +const Link = imports.ui.link; +const Panel = imports.ui.panel; +const Tweener = imports.ui.tweener; +const Workspaces = imports.ui.workspaces; These are not used. 3) Make sure all constants are used in all places where they should apply and, conversely, use constants for all the spacing and padding values. You've added quite a few random spacing values and removed some that used a constant. For example, DASH_SECTION_PADDING is now only used in one place, but it needs to be used as an actual padding first for its usage to make sense. DASH_MIN_WIDTH is not used 4) + this.emit('open-state', this._open); Maybe rename to 'open-state-changed' ? 6) Why does ResultsArea need to care about navArea instead of GenericDisplay placing it internally? If there is a reason this is necessary, perhaps GenericDisplay can return a value for enableNavigation instead of it being passed in to ResultArea? Is making the pane aware of the navArea related to the different format for displaying the search results which only the dash has information on? 7) + this.moreLink = new MoreLink(title); MoreLink doesn't take any arguments. 8) - this._informationButton.actor.x = availableWidth - ITEM_DISPLAY_PADDING_RIGHT - INFORMATION_BUTTON_SIZE; - this._informationButton.actor.y = ITEM_DISPLAY_HEIGHT / 2 - INFORMATION_BUTTON_SIZE / 2; + let buttonBox = new Big.Box({ width: INFORMATION_BUTTON_SIZE+8, height: INFORMATION_BUTTON_SIZE, + padding_left: 4, padding_right: 4, + y_align: Big.BoxAlignment.CENTER }); Need spaces in INFORMATION_BUTTON_SIZE+8. Will likely need to set x for the button so that it has appropriate padding on the right. 9) // Selects the item by highlighting it and displaying its details - this.emit('select'); + this.emit('details'); How about a more action-oriented name like 'show-details'? 10) - let largePreviewIcon = this._createLargePreviewIcon(availableWidth, Math.max(0, availableHeight - mainDetails.height - PREVIEW_BOX_SPACING)); + // Just use width for both here - it's really unlikely that width < height + let largePreviewIcon = this._createLargePreviewIcon(availableWidth, availableWidth); This will limit the height available for the vertical images. Just use -1 if you don't want to limit height. Is there a problem with passing in availableHeight to this function? 11) // Container to hold the dash as well as any popup pane chrome. When // a pane is displayed, we use this catch clicks outside of the dash panes // and the workspaces area should not be reactive. Catching such a // click results in the panes being closed and the workspaces area becoming reactive again. How about changing it to this? // Container to hold the dash as well as any popup pane chrome. When // a pane is displayed, we make the workspaces are not reactive and use // this actor to catch clicks outside of the panes. Catching such a // click results in the panes being closed and the workspaces area becoming reactive again. 12) + // Close any active panes if a GenericDisplayItem is being // Closes any active panes if a GenericDisplayItem is being... Just to be consistent with most of the function headers :). 13) There are still some trailing whitespaces: [mazik@mazik gnome-shell]$ git am ~/patch-dash-rewrite2.txt Applying: Rewrite Dash, remove hardcoded width/height from GenericDisplay /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:189: trailing whitespace. /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:675: trailing whitespace. /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:689: trailing whitespace. /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:693: trailing whitespace. /home/mazik/gnome-shell/source/gnome-shell/.git/rebase-apply/patch:696: trailing whitespace. warning: squelched 10 whitespace errors warning: 15 lines add whitespace errors.
Created attachment 139836 [details] [review] Rewrite Dash, remove hardcoded width/height from GenericDisplay This patch is a near-total rewrite of the Dash. First, the dash code moves into a separate file, dash.js. Inside dash.js, the components are more broken up into separate classes; in particular there's now a Pane class and a MoreLink class. Instead of each section of the dash, when activated, attempting to close all N-1 other sections, instead there is the concept of a single "active pane", and when e.g. activating the More link for documents, if we know there's an active pane which happens to be the apps, close it. Many redundant containers were removed from the dash, and all manual width, height and x/y offsets are entirely gone. We move the visual apperance closer to the design by using the view-more.svg, etc. To complete the removal of height/width calculations from the dash, we also had to do the same for GenericDisplay.
> 1) Dash is missing the background color. > 2) Did you remove the gradient for the dash and panes intentionally? I thought it was a bit nicer with it. I don't see any color/gradient in the older mockups, but we can add something. I did remove it intentionally because it was a lot of code duplication. If we wanted to readd it now it could be done in one place inside Pane. > Probably need to remove get_max_word_width() function itself if we don't intend on using it. Yeah, though this stuff is in bug 588474 and I don't want to conflict more. It'll be removed by that bug. > 3) Make sure all constants are used in all places where they should apply and, conversely, use constants for all the spacing and padding values. You've added quite a few random spacing values and removed some that used a constant. Well....I'm unhappy about adding tons of constants when really we should be switching to CSS. My suggestion here is that we use constants for the key stuff, but I just used a DEFAULT_PADDING = 4 for various paddings and spacings. In the end we're going to have to git grep for padding: and spacing: and fix all this stuff later. Everything else should be fixed; you can use "interdiff old.patch new.patch" to see the difference between the patches.
Created attachment 139838 [details] [review] Rewrite Dash, remove hardcoded width/height from GenericDisplay This patch is a near-total rewrite of the Dash. First, the dash code moves into a separate file, dash.js. Inside dash.js, the components are more broken up into separate classes; in particular there's now a Pane class and a MoreLink class. Instead of each section of the dash, when activated, attempting to close all N-1 other sections, instead there is the concept of a single "active pane", and when e.g. activating the More link for documents, if we know there's an active pane which happens to be the apps, close it. Many redundant containers were removed from the dash, and all manual width, height and x/y offsets are entirely gone. We move the visual apperance closer to the design by using the view-more.svg, etc. To complete the removal of height/width calculations from the dash, we also had to do the same for GenericDisplay. Also clean up the positioning inside overlay.js so calculation of children's positioning is inside a single function that flows from screen.width and screen.height, so in the future we can stop passing the width into the Dash constructor and call this once and work on screen resizing.
There is still a number of regressions. Can you please test your changes against all the items below before attaching a new patch. I'll review the code then. We might have to do another rewrite of the overlay components based on a new iteration of the design, so there are some regressions that it's ok to keep for now in the areas that will likely be affected by the new design, such as search. I'm listing these regressions separately. Issues that need to be fixed: 1) I get this error when dragging an item. JS ERROR: !!! Exception was: TypeError: this._activeDisplayPane is null JS ERROR: !!! lineNumber = '227' JS ERROR: !!! fileName = '/home/mazik/gnome-shell/source/gnome-shell/js/ui/overlay.js' JS ERROR: !!! stack = '([object Object],[object _private_Clutter_Texture],NaN,NaN,1155744)@/home/mazik/gnome-shell/source/gnome-shell/js/ui/overlay.js:227 ([object _private_Clutter_Texture],[object _private_Clutter_Event])@/home/mazik/gnome-shell/source/gnome-shell/js/ui/dnd.js:138 ([object _private_Clutter_Texture],[object _private_Clutter_Event])@/home/mazik/gnome-shell/source/gnome-shell/js/ui/dnd.js:64 ([object _private_Clutter_Texture],[object _private_Clutter_Event])@/home/mazik/gnome-shell/install/share/gjs-1.0/lang.js:110 ' JS ERROR: !!! message = 'this._activeDisplayPane is null' 2) Dropping an item doesn't work in certain areas of the overlay. This might be dependent on the dash width. We had a bug just like that right before GUADEC, so you can look at the commits from then to get an idea. 3) Clicking in the workspaces area of the overlay when a pane is being shown closes the overlay. It should only close the pane instead. 4) Search seems to be taking longer and shows "intermediate" results before showing the results for the final letter combination. Must have something to do with how we schedule / cancel the search tasks. There is also a second pane that shows up for search results when one of the "More" panes is out, but the "More" pane disappears only when the search results are done showing. 5) Search results are missing section headers. 6) Close button has a strange highlight on mouse-over. Regressions that it's ok to keep in view of possible changes in the design: 1) Dash is missing the background color. 2) The gradient is not being used. (You addressed that in your comments.) 3) Details pane is being shown in the same place as the results pane with no possible navigation back to the results pane. 4) Clicking More when viewing search results should narrow down the results to that type of items. Conversely, search should apply only to a particular type of items when More is already selected. 5) "More" pane for documents is too wide.
Created attachment 139904 [details] [review] Rewrite Dash, remove hardcoded width/height from GenericDisplay This patch is a near-total rewrite of the Dash. First, the dash code moves into a separate file, dash.js. Inside dash.js, the components are more broken up into separate classes; in particular there's now a Pane class and a MoreLink class. Instead of each section of the dash, when activated, attempting to close all N-1 other sections, instead there is the concept of a single "active pane", and when e.g. activating the More link for documents, if we know there's an active pane which happens to be the apps, close it. Many redundant containers were removed from the dash, and all manual width, height and x/y offsets are entirely gone. We move the visual apperance closer to the design by using the view-more.svg, etc. To complete the removal of height/width calculations from the dash, we also had to do the same for GenericDisplay. Also clean up the positioning inside overlay.js so calculation of children's positioning is inside a single function that flows from screen.width and screen.height, so in the future we can stop passing the width into the Dash constructor and call this once and work on screen resizing.
> 2) Dropping an item doesn't work in certain areas of the overlay. What items and what area? > 4) Search seems to be taking longer and shows "intermediate" results before showing the results for the final letter combination. Not sure about longer, but AFAIK we've always shown results while typing, which implies intermediate results. Everything else in the list should be fixed.
(In reply to comment #12) > > 2) Dropping an item doesn't work in certain areas of the overlay. > > What items and what area? It looks like it got fixed in your last patch. Basically, it was not possible to drop application and document items on the left-hand side of the workspaces area. > > > 4) Search seems to be taking longer and shows "intermediate" results before showing the results for the final letter combination. > > Not sure about longer, but AFAIK we've always shown results while typing, which > implies intermediate results. Right, it is comparably slow before your changes. Probably because I have too many recent documents. Showing the More pane and the pane with results next to each other and then removing the More pane is still a problem, but it's a minor one. > Everything else in the list should be fixed. Yep, looks good. I'll take a look at the code.
Just a few small comments. You can commit the patch once you apply them. +Pane.prototype = { + _init : function () { You have an extra space in function headers in many places. +Pane.prototype = { + _init : function () { + this._open = false; + + this.actor = new Big.Box({ orientation: Big.BoxOrientation.VERTICAL, + background_color: PANE_BACKGROUND_COLOR, + border: PANE_BORDER_WIDTH, + border_color: PANE_BORDER_COLOR, + padding: DEFAULT_PADDING, + reactive: true }); You need to call this.actor.hide(); here. This is important for the panes that get added, but should stay hidden until they are explicitly opened, such as the search pane. +function createPaneForDetail(dash, display, detailsWidth) { How about calling this function createPaneForDetails? +function ResultPane(dash, detailWidth) { Should be called detailsWidth throughout for consistency. + this.entry.connect('text-changed', Lang.bind(this, function (e) { + let text = this.entry.text; + })); I don't think this is needed anymore. + setPane: function (pane) { + this.pane = pane; Perhaps if this.pane should not be set directly call it this._pane ? +SectionHeader.prototype = { + _init : function (title) { + this.actor = new Big.Box({ orientation: Big.BoxOrientation.HORIZONTAL, + spacing: 6 }); Perhaps we don't need spacing here? We are appending one item to the front here and one item to the end, and it is unlikely they will overlap.
> You have an extra space in function headers in many places. Hmm true...but I think the codebase in general is very inconsistent here now, just run: git grep ' : function' Fixed everything else and pushed, thanks for the review!