Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HackStudio: Opening multiple files simultaneously makes it freak out #16263

Open
AtkinsSJ opened this issue Nov 30, 2022 · 5 comments
Open

HackStudio: Opening multiple files simultaneously makes it freak out #16263

AtkinsSJ opened this issue Nov 30, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@AtkinsSJ
Copy link
Member

Peek 2022-11-30 11-47

To reproduce:

  1. Open HackStudio
  2. Select multiple files using shift
  3. Right-click and "Open"
  4. Gaze in wonder as it rapidly switches between open files, without you doing anything, forever. (This happens faster in real life than the gif shows.)
@AtkinsSJ AtkinsSJ added the bug Something isn't working label Nov 30, 2022
@0xdeadbeer
Copy link
Contributor

Started analyzing the code couple of hours ago, a simple solution to this in my opinion would be to disable multi-selection inside the treeview?

m_project_tree_view->set_selection_mode(GUI::AbstractView::SelectionMode::MultiSelection);


Else I'm pretty sure there's a problem with how the create_open_selected_action action is opening those multiple files..

NonnullRefPtr<GUI::Action> HackStudioWidget::create_open_selected_action()
{
auto open_selected_action = GUI::Action::create("&Open", [this](const GUI::Action&) {
auto files = selected_file_paths();
for (auto& file : files)
open_file(file);
});
open_selected_action->set_icon(Gfx::Bitmap::try_load_from_file("/res/icons/16x16/open.png"sv).release_value_but_fixme_should_propagate_errors());
open_selected_action->set_enabled(true);
return open_selected_action;
}

@0xdeadbeer
Copy link
Contributor

At the end I've found out that set_activate_widget is responsible for such behavior.

void HackStudioWidget::add_new_editor(GUI::TabWidget& parent)
{
auto& wrapper = parent.add_tab<EditorWrapper>("(Untitled)");
parent.set_active_widget(&wrapper);
if (parent.children().size() > 1 || m_all_editor_tab_widgets.size() > 1)
parent.set_close_button_enabled(true);

Don't really know its purpose (not even after trying to read the function) so I'm not sure whether removing the call is the best fix.

@MacDue
Copy link
Member

MacDue commented Dec 5, 2022

Started analyzing the code couple of hours ago, a simple solution to this in my opinion would be to disable multi-selection inside the treeview?

That sounds like just papering over the issue, I think it's valid to want to open multiple files at once.

@MacDue
Copy link
Member

MacDue commented Dec 5, 2022

@osamu-kj I think you're close to a fix (I've not tested this), but I think the issue is probably in the deferred invoke here, that sets the current editor:

set_current_editor_wrapper(tab);

You probably want to debounce setting the active editor (so it only ends up switching to the last file), you might be able to do something with Core::debounce() there (or some special logic).

@0xdeadbeer
Copy link
Contributor

hmm, I'm missing out on what some of the code over there does..
Isn't on_tab_close_request supposed to be called when the user closes a tab?
Why are there so many functions that seem like they do the same thing on the first glance? (set_current_editor_wrapper, set_active_widget, set_focus)? I think I'll have to go much deeper into the code later on today..

MacDue added a commit to MacDue/serenity that referenced this issue Dec 16, 2022
Wanted to look at this one since it looked quite funny :^)

This seemed to come from an awkward interplay of deferred callbacks
and doubly deferred callbacks! This is fixed here my simply avoiding
making some of these calls in the first place (as they were unnecessary
anyway).

But the infinite loop came from something like this:

set_tab(A)        // Active tab A
defer set_tab(A)  // from on_change()
set_tab(B)        // Active tab B
defer set_tab(B)  // from on_change()

-> Deferred invokes run:

set_tab(A)        // Tab changed from B -> A
defer set_tab(A)  // fire on_change() -- queue new defer
set_tab(B)        // Tab changed from A -> B
defer set_tab(B)  // fire on_change() -- queue new defer

:infinteyakloop:

Fixes SerenityOS#16263
MacDue added a commit to MacDue/serenity that referenced this issue Dec 16, 2022
Wanted to look at this one since it looked quite funny :^)

This seemed to come from an awkward interplay of deferred callbacks
and doubly deferred callbacks! This is fixed here by simply avoiding
making some of these calls in the first place (as they were unnecessary
anyway).

But the infinite loop came from something like this:

set_tab(A)        // Active tab A
defer set_tab(A)  // from on_change()
set_tab(B)        // Active tab B
defer set_tab(B)  // from on_change()

-> Deferred invokes run:

set_tab(A)        // Tab changed from B -> A
defer set_tab(A)  // fire on_change() -- queue new defer
set_tab(B)        // Tab changed from A -> B
defer set_tab(B)  // fire on_change() -- queue new defer

:infinteyakloop:

Fixes SerenityOS#16263
MacDue added a commit to MacDue/serenity that referenced this issue Dec 17, 2022
Wanted to look at this one since it looked quite funny :^)

This seemed to come from an awkward interplay of deferred callbacks
and doubly deferred callbacks! This is fixed here by simply avoiding
making some of these calls in the first place (as they were unnecessary
anyway).

But the infinite loop came from something like this:

set_tab(A)        // Active tab A
defer set_tab(A)  // from on_change()
set_tab(B)        // Active tab B
defer set_tab(B)  // from on_change()

-> Deferred invokes run:

set_tab(A)        // Tab changed from B -> A
defer set_tab(A)  // fire on_change() -- queue new defer
set_tab(B)        // Tab changed from A -> B
defer set_tab(B)  // fire on_change() -- queue new defer

:infinteyakloop:

Fixes SerenityOS#16263
MacDue added a commit to MacDue/serenity that referenced this issue Dec 17, 2022
Wanted to look at this one since it looked quite funny :^)

This seemed to come from an awkward interplay of deferred callbacks
and doubly deferred callbacks. This is now fixed with a workaround to
prevent `current_editor().set_focus(true)' from updating the editor
if the current editor changed by the time the (deferred) focus event
fires.

But the infinite loop came from something like this:

set_tab(A)        // Active tab A
defer set_tab(A)  // from on_change()
set_tab(B)        // Active tab B
defer set_tab(B)  // from on_change()

-> Deferred invokes run:

set_tab(A)        // Tab changed from B -> A
defer set_tab(A)  // fire on_change() -- queue new defer
set_tab(B)        // Tab changed from A -> B
defer set_tab(B)  // fire on_change() -- queue new defer

:infinteyakloop:

Fixes SerenityOS#16263
MacDue added a commit to MacDue/serenity that referenced this issue Dec 17, 2022
Wanted to look at this one since it looked quite funny :^)

This seemed to come from an awkward interplay of deferred callbacks
and doubly deferred callbacks. This is now fixed with a workaround to
prevent `current_editor().set_focus(true)' from updating the editor
if the current editor changed by the time the (deferred) focus event
fires.

But the infinite loop came from something like this:

set_tab(A)        // Active tab A
defer set_tab(A)  // from on_change()
set_tab(B)        // Active tab B
defer set_tab(B)  // from on_change()

-> Deferred invokes run:

set_tab(A)        // Tab changed from B -> A
defer set_tab(A)  // fire on_change() -- queue new defer
set_tab(B)        // Tab changed from A -> B
defer set_tab(B)  // fire on_change() -- queue new defer

:infinteyakloop:

Fixes SerenityOS#16263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants