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

Fix for issue #1122 #1661

Merged
merged 17 commits into from Oct 8, 2012
Merged

Fix for issue #1122 #1661

merged 17 commits into from Oct 8, 2012

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Sep 16, 2012

This pull request provides a possible fix for issue #1122.

It adds resize functionality for the two existing bottom panels ("jslint errors" and "find in files").

Knwon issues

  • If a panel is resized beyond the main toolbar, it keeps growing. If the mouse is released within the toolbar the panel height is greater than the main view height. When starting the panel resize again, there is an offset between the handler and the panel height due to this.
  • If both panels are opened, resizing the search panel until both the panels cover the entire editor and keep going causes the same effect as the previous issue.
  • If both panels are opened, resizing the jslint panel beyond the main toolbar causes the find panel to shift downwards as the first one keeps growing.

This issues could be solved but it will require to introduce some dependencies between panels or dom elements and betwen panels and the toolbar element.

@ghost ghost assigned redmunds Sep 17, 2012
@redmunds
Copy link
Contributor

Thanks for submitting this code. We have an internal milestone that we're trying to hit this week, so I might not get to reviewing this code until next week.

@redmunds
Copy link
Contributor

This is cool, but I'm seeing a couple problems with this on Win7. I posted a pic here: https://twitter.com/randyedmunds/status/250976078926077952/photo/1 (Let me know if the resolution got messed up and I can e-mail the pic).

Notice in the upper (JSLint) panel that after increasing the height of the panel, I still see the same number of lines of JSLint errors displayed (with a vertical scrollbar). The number of lines viewed should be increased to fill the height of the panel.

Notice the lower (Find in Files) panel that the number of lines are increased with the height, but on the lower end of the scrollbar, the down-arrow button is mostly off the screen.

…gins

Fixed missing toolbar.outerHeight() in _setHeight for jsLint
@jbalsas
Copy link
Contributor Author

jbalsas commented Sep 26, 2012

Hi!

I think I messed up the jslint results panel resize when doing some cleaning before the pull request... fixed now :)

About the other issue, scrollbars on Mac are somehow subtler so this was harder to spot than on windows. I was using height() to get the toolbars height, which wasn't considering paddings and margins. I've changed it to outerHeight() instead. Please, check it to see if is ok now.

@redmunds
Copy link
Contributor

Excellent. Both of the problems mentioned above are fixed. I am noticing another problem with releasing mouse capture. Do this:

  1. Open JSLint panel (it only happens with JSLint panel, either by itself or with FiFs panel)
  2. Drag height up as high as you can (until editor disappears)
  3. Release left mouse button
  4. Now move mouse down over JSLint panel

Results: JSLint panel height starts decreasing as if mouse button was never released, so it seems to still have the capture.

Expected: Obviously, releasing the mouse should stop the drag, but I'm not sure if I like how the entire editor can be covered up. I am wondering if enforcing a minimum height on the editor will fix both problems?

@@ -50,5 +50,6 @@

@z-index-brackets-sidebar-resizer: @z-index-brackets-ui + 2;
@z-index-brackets-resizer-div: @z-index-brackets-sidebar-resizer + 1;
@z-index-brackets-bottom-resizer: @z-index-brackets-ui + 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@z-index-brackets-bottm-panel-resizer would be more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix...

@jbalsas
Copy link
Contributor Author

jbalsas commented Sep 27, 2012

I basically thought of this as a temporary fix while getting a crack at the tabbed panel solution discussed in https://groups.google.com/forum/?fromgroups=#!topic/brackets-dev/PqEbuhw7k6c. I feel, that would be the best way to go.

We should of course improve this if it is to be made part of Brackets in the meantime.

About the behavior you saw, I think it only happens when you release the mouse outside the Brackets window (also over the app toolbar). It also happens for the FiFs panel. I'll try to fix this later today.

About enforcing a minimum height for the editor, I did try it, but it didn't convince me at all. There are lots of issues to be addressed for that:

1. Dependencies between panels

We'd need to check the height of all visible panels to get the available space. I didn't want to create any dependencies between panels. Having a utils/Resizer class that tracks all panels could help with this...

2. Resizing the window

  • With one panel

If we start resizing the whole window... what should happen when the minimum height for the editor is reached? Should the panel shrink to leave place for the editor?

  • With two or more panels

If we start resizing the whole window, when reaching the minimum height for the editor, should the panels shrink? Should all panels shrink in the same proportion? Ones more than others?

3. Resizing the panels

If we start resizing a panel and the maximum height for all panels is reached, should we stop all resizing or should other panels give height away for or it?

4. Opening panels

If we try to open a panel and there is not enough space for the saved height of the panel, should we set it to the maximum available height? What if no space at all is left?

I think solving all these issues may be a lot of work that could be better spent on the tabbed panel solution if that is the final goal. I'm willing to work in both directions in any case, so just point at what should we do, please :)

@redmunds
Copy link
Contributor

This resizing code will still be needed if the tabbed interface is implemented.

Enforcing a minimum height on the editor is just a suggestion for a possible solution, but it's not required. I does not seem to be worth the effort.

But, the mouse not releasing the capture needs to be solved. Can the "dragleave" event be used to release the mouse at the point when mouse exits window?

@jbalsas
Copy link
Contributor Author

jbalsas commented Sep 27, 2012

I think dragleave is for drag operations, which we aren't really doing. However, jquery .mouseleave() should do the trick.(Note that this is currently also happening for the sidebar...)

I'll check and fix this and refactor the resize as suggested.

@jbalsas
Copy link
Contributor Author

jbalsas commented Oct 1, 2012

I've created a a new utils/Resizer as discussed.

It scans the DOM on AppInit.htmlReady for ver-resizable and hor-resizable classes and adds the resize handlers for those elements. It expects a top-resizer or bottom-resizer class in the case of vertical resizing to identify where to put the handler (both could work at the same time in the future). The same way, it expects a left-resizer or right-resizer for horizontal resizing. Finally, it also looks for an optional element with a content-resizable class that should be resized at the same time the parent does, to fill the available space.

The process creates a promise that the resizable modules can use to get notifications of the resizing process using the progress method. The associated deferred objetct never gets resolved. It gets notified any time the resize starts, updates or ends. This way, modules can do performance improvements (like the sidebar does, hiding elements when resizing starts), custom resize on progress, or saving preferences and restoring elements when resize ends.

Known Issues

  • The code works right now for vertical resizing and resizer on top. Bottom vertical resizing as well as horizontal resizing would need small modifications. I think this could be done in future requests.
  • The scan for classes is triggered on AppInit.htmlReady, and modules can retrieve their resize promises on AppInit.appReady. This prevents extensions to benefit from this and force them to explicitly call makeResizable. We could solve this by triggering the scan on AppInit.appReady instead and create a new event Resizer.ready or AppInit.resizerReady to be triggered afterwards.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2012

Excellent! This is much better. Now that the high-level design is in place, I'm going to do one last review of the code details.

@@ -23,7 +23,7 @@


/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, $, brackets, JSLINT, PathUtils */
/*global define, $, JSLINT, PathUtils, document, window */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like both document and window are no longer used and can be removed

@@ -0,0 +1,200 @@
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy the Adobe copyright header comment from any other .js file and paste it at the top.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2012

Done with review.

…t-panel-resize

Conflicts:
	src/brackets.js
	src/language/JSLintUtils.js
	src/search/FindInFiles.js
@redmunds
Copy link
Contributor

redmunds commented Oct 8, 2012

@jbalsas I see you pushed some changes since my last comment, so I am just checking to see where we're at with this one. I want to get this change into master so I can use it! Also, there are conflicts with master, so you'll need to merge the latest changes to master to this branch. Thanks.

@jbalsas
Copy link
Contributor Author

jbalsas commented Oct 8, 2012

@redmunds Yes, I added your final suggestions the other day in London (@peterflynn helped me with those ;)) Please, tell me if there's anything left to add.

I'll merge the changes to master later tonight.

…t-panel-resize

Conflicts:
	src/search/FindInFiles.js
@redmunds
Copy link
Contributor

redmunds commented Oct 8, 2012

@jbalsas Glad I asked :) Please post a comment with @redmunds when passing something back so I don't wait unnecessarily.

This looks good. 2 very minor other changes:

  • line 190 of Resizer.js has a typo -- it should be "right" not "bottom".
  • Totally optional, but do you mind renaming "ver-resizable" to be "vert-resizable", and "hor-resizable" to be "horz-resizable"? Seems much quicker to remember what they're for.

@jbalsas
Copy link
Contributor Author

jbalsas commented Oct 8, 2012

@redmunds Done and done ;)

@redmunds
Copy link
Contributor

redmunds commented Oct 8, 2012

Looks good. Thanks for sticking with this one! Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants