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

Fix relative paths with the Particle Editor #3137

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

Fix relative paths with the Particle Editor #3137

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
7 changes: 6 additions & 1 deletion src/editor/editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,12 @@ Editor::update(float dt_sec, const Controller& controller)
m_particle_editor_request = false;
std::unique_ptr<Screen> screen(new ParticleEditor());
if (m_particle_editor_filename)
static_cast<ParticleEditor*>(screen.get())->open("particles/" + *m_particle_editor_filename);
{
// realpath() is necessary for particle files outside the particles/ folder
std::string path = physfsutil::realpath("particles/" + *m_particle_editor_filename);
Copy link
Member

Choose a reason for hiding this comment

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

Hey, whatever happened to FileSystem::join? Or std::filesystem::path::operator/? Not the only place where this happens so if you're gonna fix this then do it there aswell

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly to above, I have opted not to change the code more than necessary to keep the changes minimal and avoid going against eventual preferences. The join function also does not normalize the path, so it would be an extra call rather than a replacement.


static_cast<ParticleEditor*>(screen.get())->open(path);
Copy link
Member

Choose a reason for hiding this comment

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

You really need to static cast here? You can store the new ParticleEditor in a variable and use that. In my opinion, that would be more readable. And then you can std::make_unique in the push_screen function

Copy link
Member Author

Choose a reason for hiding this comment

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

The static cast was already there. I've copied the code and changed as little as possible so as not to transform the code too much, since I'm not very familiar with the current coding standards and people's preferences. A static cast will be needed either way if screen's type is a pointer to Scene; I can move the static cast on its own line but I can't really avoid it without changing more code. I can't make_unique the screen directly in push_screen because I need to call the open function on the scene before pushing it.

}
ScreenManager::current()->push_screen(std::move(screen));
return;
}
Expand Down
6 changes: 5 additions & 1 deletion src/object/custom_particle_system_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "editor/editor.hpp"
#include "gui/menu_manager.hpp"
#include "physfs/util.hpp"
#include "util/reader.hpp"
#include "util/reader_document.hpp"
#include "util/reader_mapping.hpp"
Expand Down Expand Up @@ -66,7 +67,10 @@ CustomParticleSystemFile::update_data()
{
try
{
auto doc = ReaderDocument::from_file("particles/" + ((m_filename == "") ? "default.stcp" : m_filename));
// realpath() is necessary for particle files outside the particles/ folder
std::string path = physfsutil::realpath("particles/" + ((m_filename == "") ? "default.stcp" : m_filename));

auto doc = ReaderDocument::from_file(path);
auto root = doc.get_root();
auto mapping = root.get_mapping();

Expand Down
6 changes: 5 additions & 1 deletion src/supertux/menu/particle_editor_menu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "gui/menu_filesystem.hpp"
#include "gui/menu_item.hpp"
#include "gui/menu_manager.hpp"
#include "physfs/util.hpp"
#include "supertux/level.hpp"
#include "supertux/gameconfig.hpp"
#include "supertux/menu/menu_storage.hpp"
Expand Down Expand Up @@ -107,8 +108,11 @@ ParticleEditorMenu::menu_action(MenuItem& item)
"/particles",
true,
[](const std::string& new_filename) {
ParticleEditor::current()->open("/particles/" +
// realpath() is necessary for particle files outside the particles/ folder
std::string path = physfsutil::realpath("/particles/" +
ParticleEditor::current()->m_filename);

ParticleEditor::current()->open(path);
MenuManager::instance().clear_menu_stack();
}
));
Expand Down
9 changes: 8 additions & 1 deletion src/supertux/menu/particle_editor_open.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "gui/dialog.hpp"
#include "gui/menu_item.hpp"
#include "gui/menu_manager.hpp"
#include "physfs/util.hpp"
#include "supertux/level.hpp"
#include "supertux/gameconfig.hpp"
#include "supertux/menu/menu_storage.hpp"
Expand Down Expand Up @@ -55,11 +56,17 @@ ParticleEditorOpen::~ParticleEditorOpen()
void
ParticleEditorOpen::menu_action(MenuItem& item)
{
std::string path;
Copy link
Member

Choose a reason for hiding this comment

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

What? Can't you just make the variable inside the switch case?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement

The coding style documentation does not specify whether to declare variables outside the switch statement or to create a new scope within the case statement. Since I'm used to the former with SDL and I couldn't find any source as to how to format a scope within a case, I put the variable outside the block. I can move it inside and create a scope, if you could provide me with a preferred formatting template.


switch (item.get_id())
{
case MNID_OPEN:
std::replace(m_filename.begin(), m_filename.end(), '\\', '/');
ParticleEditor::current()->open("/particles/" + m_filename);

// realpath() is necessary for particle files outside the particles/ folder
path = physfsutil::realpath("/particles/" + m_filename);

ParticleEditor::current()->open(path);
MenuManager::instance().clear_menu_stack();
break;

Expand Down
Loading