Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Bottom panels get messed up if they are resized to the maximum height #2015

Closed

Description

@gruehle

[Edit: The behavior as described in these steps was fixed in #2223. However, there are related issues that still remain. Please read all bug comments before closing.]

Steps to repro:

  1. Open the Getting Started project (or any project, for that matter)
  2. Do a "Find in Files" for "body"
  3. Resize the results panel as tall as it can go. There should still be a few pixels of editor viewable, but no text will be shown.
  4. Close the panel
  5. Do another "Find in Files" operation

Results:
The search results panel ends up overlapping the status bar and cannot be resized. The only way to get the panel back is to trash prefs.

Activity

  1. jbalsas commented on Nov 2, 2012

    @jbalsas
    Contributor

    Hi,

    A couple things I've noticed about this:

    1. You can actually get the panel back, but you need to do it backwards. Start making it smaller until the height of the panel is less than the editor's height and the panel will pop back up again. (This should make debugging this less painful...)
    2. Note how as the panel keeps growing the status bar starts sliding down once it has reached the maximum height.
    3. The same error can be reproduced by (steps 2 and 3 can be switched in order):
      • Resize the panel to any given height.
      • Close the panel
      • Resize Brackets window so that the height is smaller than the panel.
      • Reopen the panel

    It looks like some css or layout changes might help here. For example, the data-forcemargin for the sidebar resizer could be updated to push the status bar the same way we do with the content if required.

    Also limiting a maximum height while resizing the panels (on resizing and on showing) helps, but this would probably require a some API changes in Resizer.

  2. pthiess commented on Nov 6, 2012

    @pthiess
    Contributor

    Reviewed

  3. peterflynn commented on Nov 19, 2012

    @peterflynn
    Member

    There are several intertwined bugs here:

    1. Resizer allows the panel to be resized past the largest size that will fit (i.e. the size at which the editor is 0 height). The repro steps above almost guarantee this, since it's hard to stop your drag perfectly the instant editor height reaches 0. You'll typically overshoot a little.
    2. EditorManager._calcEditorHeight() returns a negative number when the panel is too large. jQuery.height() no-ops on negative numbers (not sure why it doesn't clip to zero instead but hey, since when do jQuery APIs make any sense?). So this causes the Editor to not get resized at all when an overly-large panel is reshown later -- resulting in the nasty behavior below where the panel is barely visible and can't be fixed via resizing (since it's already too tall), and the status bar is pushed off the bottom.
    3. If you make the panel huge, close it, resize the Brackets window smaller, then reopen the panel, it will now have a size that can't fit in the reduced available space. The panel will stick pretty far off the bottom, so the bottom of its scrollbar (if any) will be inaccessible and the status bar will be completely hidden. Also, to resize the panel you have to drag pretty far before it has any visible effect (because of how much was sticking off the bottom).
    4. If you make the panel huge, leave it open, and resize the Brackets window smaller you'll see all the same bugs as (3).

    So as far as fixes:

    • The nastiest parts of (2) are easy to fix by adding a Math.max() on the return value in _calcEditorHeight() -- the panel will then be positioned at the top as expected and resizing it will work properly (though the panel will be a tiny bit too tall from the overshoot, clipping the bottom few px of the status bar off the bottom -- a much less severe glitch).
    • (3) and the remainder of (2) can be fixed by calculating the max available space in the Resizer "show" helper and capping the panel size accordingly. We then also have to change FindInFiles to use Resizer.show()/hide() instead of the generic jQuery function (and ditto for JSLintUtils).
    • (4) would require doing the same check during window resize, which may be trickier to do in a performant way... and perhaps should be managed more centrally through EditorManager.

    Here's the code I added to the "show" helper for (3):

                // Window size may have changed since panel last visible. Make sure it still fits
                var maxSize = $("#editor-holder").height();
                if (elementSize > maxSize) {
                    elementSize = maxSize;
                    elementSizeFunction.apply($element, [maxSize]);
                }

    This isn't ideal since it hardcodes an assumption that the panel sits in the .content vertical stack. We could add fancier logic to detect whether that's actually true or not. Might want to encapsulate some of it in EditorManager, though.

  4. peterflynn commented on Nov 19, 2012

    @peterflynn
    Member

    So for Sprint 17 since we're sort of in a crunch, I would suggest only doing my first bullet -- the simple fix in EditorManager that fixes the worst aspects of the bug.

  5. gruehle commented on Nov 19, 2012

    @gruehle
    MemberAuthor

    The simple EditorManager fix sounds good for now. As you say, this will resolve the situation where you can't get the panel back unless you trash prefs, which is the most important aspect to fix.

  6. njx commented on Nov 26, 2012

    @njx

    Leaving open for the simple fix for sprint 17. We can do more later.

  7. added a commit that references this issue on Nov 27, 2012
  8. added a commit that references this issue on Nov 27, 2012
  9. redmunds commented on Nov 27, 2012

    @redmunds
    Contributor

    FBNC @gruehle Please verify partial fix submitted in pull request #2223. When confirmed, remove FBNC and Sprint 17 tags, and leave Open.

  10. gruehle commented on Nov 27, 2012

    @gruehle
    MemberAuthor

    Yes, the worst case is now fixed. Removing FBNC and Sprint 17. Keeping Medium Priority since the remaining issues are still pretty bad.

  11. njx commented on Jan 9, 2013

    @njx

    Nominating for sprint 19.

  12. njx commented on Jan 14, 2013

    @njx

    Moving out to sprint 20--doesn't seem as high pri as other sprint 19 bugs and we're getting low on time.

  13. peterflynn commented on Feb 8, 2013

    @peterflynn
    Member

    Moving to sprint 21 after discussion -- not as important as the ongoing CMv3 issues.

  14. peterflynn commented on Feb 27, 2013

    @peterflynn
    Member

    Moving to sprint 22 since the new topcoat toolbar story will probably affect how panel layout is managed.

  15. pthiess commented on Apr 10, 2013

    @pthiess
    Contributor

    Moving to Sprint 24 as per team's decision in today's standup. @peterflynn is going to write up a proposal for this case, we'll follow up on.

  16. njx commented on Apr 24, 2013

    @njx

    Further punted. @peterflynn to write up proposal for fix

  17. added 2 commits that reference this issue on May 21, 2013
  18. peterflynn commented on May 21, 2013

    @peterflynn
    Member

    PR #3943 fully fixes (1) above. Here's a rough sketch of the remaining work:

    • To fix (3) and the remainder of (2), Resizer needs to respect the panel maxsize when showing a hidden panel. (In the $element.data("show", ...) handler we can just clip elementSize appropriately -- easy fix).
    • There's also a variant of (3) when panels are initialized at startup, restoring sizes from prefs. I think we'd have to do something inside the if (elementPrefs) block in Resizer.
    • To fix (4), PanelManager needs to reduce panel sizes during/after a window resize. Not sure what logic it should use for deciding which panels to shrink (panels can have min sizes, so we'd have to factor that in too -- and then there's the question of what to do when the window is simply too small to fit all the fixed/min-size content...).
  19. added 2 commits that reference this issue on May 22, 2013
  20. njx commented on Jun 3, 2013

    @njx

    Reviewed. Since the worst issues here are fixed, and panels were never an official feature anyway, we decided that we should call this "move to backlog", and create a card for creating an official panel API that includes things like whether we want tabs in panels, what their sizing behavior should be, etc.

  21. njx commented on Jul 8, 2013

    @njx

    Created a backlog card for bottom panel UI and linked this bug from it: https://trello.com/c/eVoQHUJj. Closing as move to backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions