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

Linux/BSD platform: Change export name and fix bsd feature tag #76996

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −14
Open

Linux/BSD platform: Change export name and fix bsd feature tag #76996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions editor/project_settings_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@
#include "scene/gui/check_button.h"
#include "servers/movie_writer/movie_writer.h"

#define FEATURE_BOX_CUSTOM_ITEM_IDX 0
#define FEATURE_BOX_ALL_ITEM_IDX 1
#define FEATURE_BOX_START_ITEM_IDX 2

ProjectSettingsEditor *ProjectSettingsEditor::singleton = nullptr;

void ProjectSettingsEditor::connect_filesystem_dock_signals(FileSystemDock *p_fs_dock) {
Expand Down Expand Up @@ -163,7 +167,7 @@ void ProjectSettingsEditor::_property_box_changed(const String &p_text) {

void ProjectSettingsEditor::_feature_selected(int p_index) {
Vector<String> t = property_box->get_text().strip_edges().split(".", true, 1);
const String feature = p_index ? "." + feature_box->get_item_text(p_index) : "";
const String feature = (p_index >= FEATURE_BOX_START_ITEM_IDX) ? "." + feature_box->get_item_text(p_index) : "";
property_box->set_text(t[0] + feature);
_update_property_box();
}
Expand All @@ -172,25 +176,26 @@ void ProjectSettingsEditor::_update_property_box() {
const String setting = _get_setting_name();
const Vector<String> t = setting.split(".", true, 1);
const String name = t[0];
const String feature = (t.size() == 2) ? t[1] : "";
bool feature_invalid = (t.size() == 2) && (t[1].is_empty());
const String feature = (t.size() == 2) ? t[1] : String();

add_button->set_disabled(true);
del_button->set_disabled(true);

bool feature_in_list = false;
if (!feature.is_empty()) {
feature_invalid = true;
for (int i = 1; i < feature_box->get_item_count(); i++) {
for (int i = FEATURE_BOX_START_ITEM_IDX; i < feature_box->get_item_count(); i++) {
if (feature == feature_box->get_item_text(i)) {
feature_invalid = false;
feature_in_list = true;
feature_box->select(i);
break;
}
}
}

if (feature.is_empty() || feature_invalid) {
feature_box->select(0);
if (feature.is_empty()) {
feature_box->select(FEATURE_BOX_ALL_ITEM_IDX);
} else if (!feature_in_list) {
feature_box->select(FEATURE_BOX_CUSTOM_ITEM_IDX);
}

if (property_box->get_text().is_empty()) {
Expand All @@ -207,7 +212,7 @@ void ProjectSettingsEditor::_update_property_box() {
type_box->select(0);
}

if (feature_invalid) {
if (!feature.is_valid_identifier() && feature != "32" && feature != "64") {
return;
}

Expand Down Expand Up @@ -298,6 +303,12 @@ void ProjectSettingsEditor::_add_feature_overrides() {
ee->get_export_platform(i)->get_platform_features(&p);
for (const String &E : p) {
presets.insert(E);

// This **cannot** be added to `get_platform_features()` or `get_preset_features()`. See GH-76996.
if (E == "linuxbsd") {
presets.insert("linux");
presets.insert("bsd");
}
Comment on lines +308 to +311
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we should keep only linuxbsd in the list (without linux, bsd and other "sub-platforms")? Project Settings UI now allows feature tags that are not in the list.

}
}

Expand All @@ -319,8 +330,10 @@ void ProjectSettingsEditor::_add_feature_overrides() {
}

feature_box->clear();
feature_box->add_item(TTR("(All)"), 0); // So it is always on top.
int id = 1;
feature_box->add_item(TTR("(Custom)"), FEATURE_BOX_CUSTOM_ITEM_IDX);
feature_box->set_item_disabled(FEATURE_BOX_CUSTOM_ITEM_IDX, true);
feature_box->add_item(TTR("(All)"), FEATURE_BOX_ALL_ITEM_IDX);
int id = FEATURE_BOX_START_ITEM_IDX;
for (const String &E : presets) {
feature_box->add_item(E, id++);
}
Expand Down
4 changes: 2 additions & 2 deletions platform/linuxbsd/export/export.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ void register_linuxbsd_exporter_types() {
void register_linuxbsd_exporter() {
Ref<EditorExportPlatformLinuxBSD> platform;
platform.instantiate();
platform->set_name("Linux/X11");
platform->set_os_name("Linux");
platform->set_name("Linux/BSD");
platform->set_os_name("LinuxBSD");
Copy link
Member

Choose a reason for hiding this comment

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

What was the reasoning behind having both name and os_name?

Copy link
Member Author

Choose a reason for hiding this comment

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

set_name() sets the name displayed in the list of platforms. set_os_name() sets rather a technical name (I did not find it displayed somewhere in the interface). Also, a feature tag is formed from os_name (converted to lower case).

Copy link
Member

Choose a reason for hiding this comment

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

The os_name was changed in #61352. So it looks like we wanted to go the opposite direction and split Linux and BSD? cc @Faless

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, Linux and BSD are not compatible systems.
Binaries for Linux do not work on BSD, and vice versa, they have different OS APIs, they are not the same platform even if they share a good amount of source code.
And we must be able to detect which platform we are in if we want gdextensions to work (and not end up trying to load a linux shared library on BSD).
As I mentioned in the other PR, we might be able to simply add another exporter dedicated to BSD.

platform->set_chmod_flags(0755);

EditorExport::get_singleton()->add_export_platform(platform);
Expand Down
10 changes: 9 additions & 1 deletion platform/linuxbsd/os_linuxbsd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,11 +491,19 @@ bool OS_LinuxBSD::_check_internal_feature_support(const String &p_feature) {
return font_config_initialized;
}
#endif

#ifndef __linux__
// `bsd` includes **all** BSD, not only "other BSD" (see `get_name()`).
if (p_feature == "bsd") {
return true;
}
#endif

if (p_feature == "pc") {
return true;
}

// Match against the specific OS (linux, freebsd, etc).
// Match against the specific OS (`linux`, `freebsd`, `netbsd`, `openbsd`).
if (p_feature == get_name().to_lower()) {
return true;
}
Expand Down