[x11] Add kdialog support, more robust detection#187
Conversation
aseprite-bot
left a comment
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| #include "base/replace_string.h" | ||
| #include "base/split_string.h" | ||
|
|
||
| #include "fmt/format.h" |
There was a problem hiding this comment.
warning: 'fmt/format.h' file not found [clang-diagnostic-error]
#include "fmt/format.h"
^| if (s_cliTool == CLITool::Unknown) { | ||
| FILE* f = popen("zenity --version", "r"); | ||
| if (f && pclose(f) == 0) { | ||
| if (std::system("kdialog --version > /dev/null 2>&1") == 0) |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
if (std::system("kdialog --version > /dev/null 2>&1") == 0)
^| if (f && pclose(f) == 0) { | ||
| if (std::system("kdialog --version > /dev/null 2>&1") == 0) | ||
| s_cliTool = CLITool::KDialog; | ||
| else if (std::system("zenity --version > /dev/null 2>&1") == 0) |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
else if (std::system("zenity --version > /dev/null 2>&1") == 0)
^
ckaiser
force-pushed
the
kdialog-kfeature-kbranch
branch
from
2cd2e45 to
c543d72
Compare
aseprite-bot
left a comment
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| if (s_cliTool == CLITool::Unknown) { | ||
| FILE* f = popen("zenity --version", "r"); | ||
| if (f && pclose(f) == 0) { | ||
| if (backend == "kdialog" || std::system("kdialog --version > /dev/null 2>&1") == 0) |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
if (backend == "kdialog" || std::system("kdialog --version > /dev/null 2>&1") == 0)
^| if (f && pclose(f) == 0) { | ||
| if (backend == "kdialog" || std::system("kdialog --version > /dev/null 2>&1") == 0) | ||
| s_cliTool = CLITool::KDialog; | ||
| else if (backend == "zenity" || std::system("zenity --version > /dev/null 2>&1") == 0) |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
else if (backend == "zenity" || std::system("zenity --version > /dev/null 2>&1") == 0)
^
| // Connection to the X11 server (the Display* pointer returned by | ||
| // XOpenDisplay()). | ||
| void* x11display = nullptr; | ||
| std::string backend = "autodetect"; |
There was a problem hiding this comment.
I'd prefer if this is an enum (probably the CLITool could be moved inside FileDialog), and we convert the string <-> enum from the Aseprite side.
There was a problem hiding this comment.
Added the enum and made the backend selection fall back to whatever it finds that is working
ckaiser
force-pushed
the
kdialog-kfeature-kbranch
branch
from
71adbbb to
ce82135
Compare
aseprite-bot
left a comment
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| if (f && pclose(f) == 0) { | ||
| s_cliTool = CLITool::Zenity; | ||
| const auto hasZenity = [] { | ||
| static bool check = std::system("zenity --version > /dev/null 2>&1") == 0; |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
static bool check = std::system("zenity --version > /dev/null 2>&1") == 0;
^| if (f && pclose(f) == 0) { | ||
| s_cliTool = CLITool::Zenity; | ||
| const auto hasZenity = [] { | ||
| static bool check = std::system("zenity --version > /dev/null 2>&1") == 0; |
There was a problem hiding this comment.
warning: variable 'check' of type 'bool' can be declared 'const' [misc-const-correctness]
| static bool check = std::system("zenity --version > /dev/null 2>&1") == 0; | |
| static bool const check = std::system("zenity --version > /dev/null 2>&1") == 0; |
| return check; | ||
| }; | ||
| const auto hasKdialog = [] { | ||
| static bool check = std::system("kdialog --version > /dev/null 2>&1") == 0; |
There was a problem hiding this comment.
warning: function is not thread safe [concurrency-mt-unsafe]
static bool check = std::system("kdialog --version > /dev/null 2>&1") == 0;
^| return check; | ||
| }; | ||
| const auto hasKdialog = [] { | ||
| static bool check = std::system("kdialog --version > /dev/null 2>&1") == 0; |
There was a problem hiding this comment.
warning: variable 'check' of type 'bool' can be declared 'const' [misc-const-correctness]
| static bool check = std::system("kdialog --version > /dev/null 2>&1") == 0; | |
| static bool const check = std::system("kdialog --version > /dev/null 2>&1") == 0; |
|
There is just that minor change, but after that I think we can merge it 👍 looks good. |
ckaiser
force-pushed
the
kdialog-kfeature-kbranch
branch
from
ce82135 to
abee677
Compare
|
Looks great 🗄️ 🚀 |
3 participants
Adds kdialog support and improves the check (popen was either racing or something else strange was happening because it was always defaulting to the built-in Aseprite dialog except when I was stepping through with the debugger to give it time, std::system will always block).