Issue #[email protected]

[dsych]

Fixed problem described in mozilla/thimble.mozilla.org#1154. Problem occurred when layout properties were loaded from cache. Editor frame was manually allocated, which was not necessary due to its dynamic nature. Only need to initialize sidebar and preview tab(second pane) for editor to fill the rest.

[gideonthomas]

@flukeout are you able to test this?

This looks like a good fix.

[flukeout]

Yes, testing now.

[flukeout]

OK, after resolving all of my terrible caching issues I was able to test this properly @dsych and here's what I think is going on...

• We only set the width of the Sidebar when we load the load the editor now
• So, the editor and preview window always take up 50% of the remaining space
• We also want to remember the width of the Editor window but this doesn't happen anymore.

That's why we had the firstPaneWidth code in there...

var firstPaneWidth = BrambleStartupState.ui("firstPaneWidth");
if(firstPaneWidth) {
  $("#first-pane").width(firstPaneWidth);
}

Which is good, because that sets the size of the Editor to what we used to have. However, the problem comes when we try to resize, by dragging the browser size - it doesn't work.

The reason seems to be that firstPaneWidth is a pixel value. When we change it to a percentage instead, the two panes resize as we expect.

So I think the solution is to keep track of BrambleStartupState.ui("firstPaneWidth") as a percentage, instead of a pixel value, and our problem should go away.

Thoughts?

Here's a test when we set the value as a percentage, you can see it resizes appropriately

[flukeout]

Check out the comments I made, changing the stored width value to a percentage should fix this and retain the original feature.

[dsych]

Fair enough @flukeout , I will change the size of the first pane to be the percentage equivalent of the total size.

[gideonthomas]

@flukeout, before we land anything, can we chat about this change over a quick call?

I feel like this might not be the right way we want to make this change. I think changing to percentages is fine, but we shouldn't let Brackets coerce percentage values for width that are set dynamically. I think Thimble should be responsible for setting an initial percentage value, and after that, Brackets should use whatever unit Thimble sets (in this case a percentage but it could easily be px, rem, etc.) and not change it. Part of this fix would involve changing the way we store the widths in localStorage (currently we get/set them as integers, we should just use strings containing the unit instead) as well as changing how we get width of the panes in a few pieces of code (we use .width() but should probably be using .css("width")).

@dsych thank you for being patient with this patch and experimenting with different ways to fix this. I think your work is getting us very close to the right fix for this issue. As @humphd mentioned, this is a hard problem to fix and has stumped us for a while now.

[dsych]

@gideonthomas, I am glad to be of help in this matter that is all. Let me know how you want to approach the problem next.

[flukeout]

@gideonthomas sure let's chat. It's just that the native feature for resizing already uses percentages. That's why it's not resizing the views when we first load, because we explicitly set widths as a pixel value, and the resizing code doesn't know how to handle it. If you inspect the code on the panes, you'll see that the widths are set in percentages by the Resizer.js code. I think that's the only issue that's been making this happen, it sets it in percentages, we store it in pixels.

[gideonthomas]

@flukeout yeah that makes sense, but I think the fix to that would be that we shouldn't be setting pixel values in the first place (based on the piece of code touched here, it is not explicitly setting a pixel value here it's just getting what was already set). So somewhere in our code we are setting a px value and we should probably change that to a percentage. Then, brackets should use the same unit throughout (and the calculation done here would not be needed).

[flukeout]

Agreed, the best solution would be to store the width as a percentage, then apply it as a percentage on load.

[dsych]

@gideonthomas @flukeout It seems like you have agreed upon something. From what I understand, you want to fix the part where widths are being set as pixels in brackets and change the unit system in thimble.
So I have two questions:

1. Do you want me to continue working on this problem?
2. If question one is true; am I understanding your intention correctly?

[gideonthomas]

@dsych yeah we just chatted. Your understanding of it is basically what I was trying to convey but after debugging with @flukeout, it seems like it's not easy to figure out where that value is being set.

So, instead, I think we'll go with what you have. However, I want to quickly test out if this condition also catches the case when we override the initial width through the Bramble API. If it does, let's land this as it is. If it doesn't, if you don't mind, I'm gonna ask you to remove the functionality from the Bramble API that allows you to override the initial value.

tldr; the core of your fix is right, we will take it in. Just let me quickly test if it catches a case I currently have in mind.

[dsych]

@gideonthomas sounds great.

[flukeout]

@dsych Your solution calculates the width of the pane based on the entire width of the editor (files + editor + preview) and then uses that percentage.

However, in the Editor the First and Second Pane are wrapped by an element - #editor-holder - so the % you set for firstPaneWidth needs to be relative to that.

I think just adding up firstPaneWidth + secondPaneWidth and figure out the percentage from that would work.

[dsych]

@flukeout Thanks for the comment, I will make the update.

[dsych]

@gideonthomas I don't know if this will help, but I seem to find the location where the StateManager is being initialized. https://github.com/mozilla/brackets/blob/master/src/bramble/client/main.js#L228

[gideonthomas]

@dsych once you make the update based on @flukeout's suggestion, I think we can land this since I can't reproduce the error on any case.

If you're interested, here's how far we came with trying to standardize the unit used ( https://github.com/gideonthomas/brackets/pull/2/files and https://github.com/gideonthomas/thimble.mozilla.org/pull/2/files). Apparently that might not work either because jQuery doesn't support what we're trying to do. So we might as well stick to the force percentages solution.

One thing I'm realizing is that we have too much state information that is changed in a lot of places at different times. We should probably try to simplify that code.

[dsych]

@gideonthomas @flukeout I will leave this standardization problem to you, since I obviously lack expertise regarding different modules of the system. I might return to it later, after getting more familiar with the system components.

[gideonthomas]

@flukeout is this good to land now?

[flukeout]

Checking now!

[flukeout]

Thanks so much for this work @dsych - works as expected!