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.

Sign up for GitHub

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

Jump to bottom

Updates filesystem dock when theme is changed #23215

Merged
merged 1 commit into from
Oct 25, 2018
Merged

Updates filesystem dock when theme is changed #23215

merged 1 commit into from
Oct 25, 2018

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Oct 22, 2018 •
edited
Loading

When theme changed filesystem dock does not properly updated, after this PR it will..

Before:

image

After:

image

@Chaosus Chaosus added this to the 3.1 milestone Oct 22, 2018
@Chaosus Chaosus changed the title Update filesystem dock when theme changed Updates filesystem dock when theme is changed Oct 22, 2018
groud
groud reviewed Oct 23, 2018
View reviewed changes
editor/filesystem_dock.cpp Outdated
display_mode = new_display_mode;
old_display_mode_setting = display_mode_setting;
if (p_force || new_display_mode != display_mode || old_display_mode_setting != display_mode_setting) {
if (!p_force) {
Copy link
Member

Choose a reason for hiding this comment

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

This if(...) is not really needed. If a change happens in the properties but p_force is true, the properties wont be changed until another useless update.

@groud
Copy link
Member

groud commented Oct 23, 2018

Isn't there a way to check whether or not the theme has changed instead of triggering the update every time a settings changed ?

@Chaosus
Copy link
Member Author

Chaosus commented Oct 23, 2018

Oh, ok @groud I changed it, now it looks better ?

@groud
Copy link
Member

groud commented Oct 23, 2018

Looks good to me.
I'm not sure I like the "p_forced" in the API, as it means that the check to know if the update should be done is split inside and outside of the function. But for now I don't have a better idea, so I think it is good to merge.

@akien-mga akien-mga merged commit 7045476 into godotengine:master Oct 25, 2018
@akien-mga
Copy link
Member

Thanks!

@Chaosus Chaosus deleted the filedock_update branch October 25, 2018 08:57
@akien-mga
Copy link
Member

I'm getting a segfault when closing the editor, I suspect this PR:

[1] /lib64/libc.so.6(+0x3a120) [0x7f3d6cc9e120] (??:0)
[2] CowData<wchar_t>::_ref(CowData<wchar_t> const&) (/home/akien/Projects/godot/godot.git/./core/cowdata.h:315)
[3] String::String(String const&) (/home/akien/Projects/godot/godot.git/./core/ustring.h:295)
[4] EditorFileSystemDirectory::get_name() (/home/akien/Projects/godot/godot.git/editor/editor_file_system.cpp:149)
[5] FileSystemDock::_create_tree(TreeItem*, EditorFileSystemDirectory*, Vector<String>&) (/home/akien/Projects/godot/godot.git/editor/filesystem_dock.cpp:61)
[6] FileSystemDock::_update_tree(Vector<String>, bool) (/home/akien/Projects/godot/godot.git/editor/filesystem_dock.cpp:224)
[7] FileSystemDock::_update_display_mode(bool) (/home/akien/Projects/godot/godot.git/editor/filesystem_dock.cpp:251 (discriminator 2))
[8] FileSystemDock::_notification(int) (/home/akien/Projects/godot/godot.git/editor/filesystem_dock.cpp:357)
[9] FileSystemDock::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/editor/filesystem_dock.h:60 (discriminator 14))
[10] Object::notification(int, bool) (/home/akien/Projects/godot/godot.git/core/object.cpp:980)
[11] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2144)
[12] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2130 (discriminator 2))
[13] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2130 (discriminator 2))
[14] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2130 (discriminator 2))
[15] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2130 (discriminator 2))
[16] Control::_propagate_theme_changed(CanvasItem*, Control*, bool) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:2130 (discriminator 2))
[17] Control::remove_child_notify(Node*) (/home/akien/Projects/godot/godot.git/scene/gui/control.cpp:443)
[18] Node::remove_child(Node*) (/home/akien/Projects/godot/godot.git/scene/main/node.cpp:1204 (discriminator 2))
[19] Node::_notification(int) (/home/akien/Projects/godot/godot.git/scene/main/node.cpp:162)
[20] Node::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/main/node.h:46 (discriminator 14))
[21] CanvasItem::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/2d/canvas_item.h:140)
[22] Control::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/gui/control.h:51)
[23] Panel::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/gui/panel.h:40)
[24] Object::notification(int, bool) (/home/akien/Projects/godot/godot.git/core/object.cpp:980)
[25] Object::_predelete() (/home/akien/Projects/godot/godot.git/core/object.cpp:387)
[26] predelete_handler(Object*) (/home/akien/Projects/godot/godot.git/core/object.cpp:2005)
[27] void memdelete<Node>(Node*) (/home/akien/Projects/godot/godot.git/./core/os/memory.h:117)
[28] Node::_notification(int) (/home/akien/Projects/godot/godot.git/scene/main/node.cpp:158)
[29] Node::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/main/node.h:46 (discriminator 14))
[30] CanvasItem::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/2d/canvas_item.h:140)
[31] Control::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/gui/control.h:51)
[32] Object::notification(int, bool) (/home/akien/Projects/godot/godot.git/core/object.cpp:980)
[33] Object::_predelete() (/home/akien/Projects/godot/godot.git/core/object.cpp:387)
[34] predelete_handler(Object*) (/home/akien/Projects/godot/godot.git/core/object.cpp:2005)
[35] void memdelete<Node>(Node*) (/home/akien/Projects/godot/godot.git/./core/os/memory.h:117)
[36] Node::_notification(int) (/home/akien/Projects/godot/godot.git/scene/main/node.cpp:158)
[37] Node::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/main/node.h:46 (discriminator 14))
[38] EditorNode::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/editor/editor_node.h:99)
[39] Object::notification(int, bool) (/home/akien/Projects/godot/godot.git/core/object.cpp:980)
[40] Object::_predelete() (/home/akien/Projects/godot/godot.git/core/object.cpp:387)
[41] predelete_handler(Object*) (/home/akien/Projects/godot/godot.git/core/object.cpp:2005)
[42] void memdelete<Node>(Node*) (/home/akien/Projects/godot/godot.git/./core/os/memory.h:117)
[43] Node::_notification(int) (/home/akien/Projects/godot/godot.git/scene/main/node.cpp:158)
[44] Node::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/./scene/main/node.h:46 (discriminator 14))
[45] Viewport::_notificationv(int, bool) (/home/akien/Projects/godot/godot.git/scene/main/viewport.h:92)
[46] Object::notification(int, bool) (/home/akien/Projects/godot/godot.git/core/object.cpp:980)
[47] Object::_predelete() (/home/akien/Projects/godot/godot.git/core/object.cpp:387)
[48] predelete_handler(Object*) (/home/akien/Projects/godot/godot.git/core/object.cpp:2005)
[49] void memdelete<Viewport>(Viewport*) (/home/akien/Projects/godot/godot.git/./core/os/memory.h:117)
[50] SceneTree::finish() (/home/akien/Projects/godot/godot.git/scene/main/scene_tree.cpp:604)
[51] OS_X11::run() (/home/akien/Projects/godot/godot.git/platform/x11/os_x11.cpp:2821)
[52] godot-git(main+0xb9) [0x117180b] (/home/akien/Projects/godot/godot.git/platform/x11/godot_x11.cpp:56)
[53] /lib64/libc.so.6(__libc_start_main+0xeb) [0x7f3d6cc8802b] (??:0)
[54] godot-git(_start+0x2a) [0x11716aa] (/home/iurt/rpmbuild/BUILD/glibc-2.28/csu/../sysdeps/x86_64/start.S:122)

I'll build with a local revert to confirm this guess.

@akien-mga
Copy link
Member

BTW that's the longest backtrace I've seen so far in Godot, looks like there's a huge mess going on when SceneTree::finish() is called.

@akien-mga
Copy link
Member

Confirmed that reverting this PR fixes the segfault.

@Chaosus
Copy link
Member Author

Chaosus commented Oct 25, 2018

Sorry, about it

@Chaosus
Copy link
Member Author

Chaosus commented Oct 25, 2018

I think _update_display_mode(true); should not be called in NOTIFICATION_THEME_CHANGED...

@akien-mga
Copy link
Member

No problem, it was not obvious from the code change that it could cause a crash. It looks like NOTIFICATION_THEME_CHANGED is a bit trigger happy...

@Chaosus Chaosus mentioned this pull request Oct 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@groud groud groud approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Chaosus @groud @akien-mga