jisaacks / MaxPane

Sublime Text plugin to quickly maximize a pane in a multi pane layout without resetting the layout.
MIT License
78 stars 9 forks source link

Crash after changing layout while MaxPane maximized #10

Closed kfriend closed 6 years ago

kfriend commented 10 years ago

I was able to recreate the crash by following these steps:

Experienced the issue w/ ST3 and OS X 10.9.3.

Not sure what the best course of action would be here, when MaxPane is maximized. Either attempt to change the hidden layout to what the user requested, disable layout changes until MaxPane is minimized, or minimize MaxPane and change the layout.

jisaacks commented 10 years ago

Thanks for reporting.

I think the best solution might be, if a pane is maximized and the user changes layout, to un-maximize the pane then change the layout. I do not know off hand though if the layout changes are commands that can be listened for.

Gabriel-p commented 8 years ago

I'm having this same issue randomly.

Is this package still maintained? Should we move to a similar one? Is there a similar one?

pykong commented 7 years ago

Has this issue been fixed? Anyone who still values a solution to this bug? I may be able to help.

Gabriel-p commented 7 years ago

I haven't encountered this issue since reporting it. Not sure if it got fixed somewhere..

deathaxe commented 7 years ago

Issue still exists. Also see https://github.com/SublimeTextIssues/Core/issues/1785

Seems @wbond knows why, but didn't have time to fix it right now.

pykong commented 6 years ago

@deathaxe Thanks for bringing the core issue to my attention. I need a couple of days until I can sift through this information and decide how to proceed.

pykong commented 6 years ago

I have found the time to go through the core issues. Indeed when offering help on this issue I was expecting the problem to MaxPane itself, yet we here we deal with a core bug.

The preferred course of action is to wait until it the bug got fixed. This issue will remain open until then.

If someone urgently needs a workaround, a plugin to shadow the built-in methods for moving views between panes could be endowed with a safety check.

@deathaxe You have mentioned you were using a two column layout. I have created a plugin to automatically set a two column layout on every window. As it works completely automatically and not requiring a dedicated window open command, it is much more convenient than Origami. In case you are interested, find it here: TwoColumns

deathaxe commented 6 years ago

... we deal with a core bug

The reason for me to file a core issue 😄

You have mentioned you were using a two column layout

Thanks, will have a look at this package. I never felt comfortable with Origami anyway - just too complicated and weird.

mg979 commented 6 years ago

I have possibly a fix for this issue. I added set_layout (to solve the OP issue) and project_manager (since it regularly gives problems with maxed panes) to the unmaximize_before variable.

I also added a check (in both calls) for a variable that must be set (and reset) by external plugins, to prevent MAX modifying the layouts. I needed it for some stuff I'm doing, since it seems that Max Pane handles also panes that have been maximized by external plugins as its own.

class MaxPaneEvents(sublime_plugin.EventListener):
    def on_window_command(self, window, command_name, args):
        unmaximize_before = ["travel_to_pane", "carry_file_to_pane",
                             "clone_file_to_pane", "create_pane",
                             "destroy_pane", "create_pane_with_file",
                             "set_layout", "project_manager"]

        if sublime.load_settings(SHARE_OBJECT).get('block_max_pane'):
            return None

        if command_name in unmaximize_before:
            window.run_command("unmaximize_pane")

        if command_name == "exit":
            # Un maximize all windows before exiting
            windows = sublime.windows()
            for w in windows:
                w.run_command("unmaximize_pane")

        return None

    def on_activated(self, view):

        if sublime.load_settings(SHARE_OBJECT).get('block_max_pane'):
            return None

        w = view.window() or sublime.active_window()
        # Is the window currently maximized?
        if w and PaneManager.isWindowMaximized(w):
            # Is the active group the group that is maximized?
            if w.active_group() != PaneManager.maxedGroup(w):
                w.run_command("unmaximize_pane")
                w.run_command("maximize_pane")
pykong commented 6 years ago

@mg979 Thanks for your input! Everyone suffering from the problem will much appreciate it. Could you make a pull request out of your suggested changes? This would make it easier to test before deciding to merge and release your additions. Thanks!

deathaxe commented 6 years ago

These changes don't fix anything for me. ST still crashes, if I try to move a file from a maximized pane to a hidden one.

mg979 commented 6 years ago

These changes don't fix anything for me. ST still crashes, if I try to move a file from a maximized pane to a hidden one.

Those change were intended to fix only the situation described by the OP, and one of mine. However I can write this in the console, with first group maxed:

view.window().set_view_index(view, 1, 0)

and nothing crashes, that is by moving the current maxed view in a hidden group as you say. Sublime 3126.

deathaxe commented 6 years ago

How about Ctrl+K, Ctrl+Up. This is the way I push views to the next group. If I do so from a maximized group, ST still crashes.

But if I understand your fix correctly, I just need to add "new_pane" command to the unmaximize_before list. ;-)

mg979 commented 6 years ago

How about Ctrl+K, Ctrl+Up. This is the way I push views to the next group. If I do so from a maximized group, ST still crashes. But if I understand your fix correctly, I just need to add "new_pane" command to the unmaximize_before list. ;-)

Yes, it crashed but you're right, after adding new_pane to that list it doesn't crash anymore. At least it's a confirm that it's a fix that could work in most cases.

mg979 commented 6 years ago

Could you make a pull request out of your suggested changes? This would make it easier to test before deciding to merge and release your additions. Thanks!

@bfelder ok I created the pull request and also included new_pane as @deathaxe said.

pykong commented 6 years ago

@mg979 Thank you! @deathaxe Can you confirm that mg979's PR fixes the issue in question when adding the "new_pane" command?

deathaxe commented 6 years ago

Yes, I do. Adding "new_pane" to the list, restores the original layout and moves the view to the next pane as expected. Did no stress test on it, but the few times, I tried no crash occurred. 💯

pykong commented 6 years ago

@deathaxe Thank you! Do we need to extend the README with instructions how to properly use the fix? If so could you give me some bullet points or even prepare a PR in this regard?

deathaxe commented 6 years ago

I think, if the piece of code from @mg979's post with "new_pane" added to the list, is merged to the repo, everything is fine. Don't think it needs some statement in the readme as it works out of the box and just fixes the crash situation. Can prep a PR.

pykong commented 6 years ago

@mg979 @deathaxe Great :-) Just merged. Thanks guys!

mg979 commented 6 years ago

@bfelder @deathaxe the only new addition that could benefit from a mention in the readme is the "block_max_pane" setting, but it has no effect unless specifically set from outside. It may be useful if somebody makes a plugin that maxes the layout, to prevent Max Pane from doing the same or reverting the changes before it's requested. It's like an undocumented feature right now.