mirror of
https://github.com/aseprite/aseprite.git
synced 2025-03-12 07:13:23 +00:00
Fix crashes using deleted items from the file selector navigation history (fix #2192)
Now if we detect that a folder doesn't exist anymore, we remove the item from the navigation history to avoid having a pointer to an invalid IFileItem.
This commit is contained in:
David Capello
parent
c1d5c94e22
commit
b5b450afba
@ -1,4 +1,5 @@
|
||||
// Aseprite
|
||||
// Copyright (C) 2019 Igara Studio S.A.
|
||||
// Copyright (C) 2001-2018 David Capello
|
||||
//
|
||||
// This program is distributed under the terms of
|
||||
@ -46,15 +47,31 @@
|
||||
#define MAX_PATH 4096
|
||||
#endif
|
||||
|
||||
#define NOTINITIALIZED "{__not_initialized_path__}"
|
||||
#define NOTINITIALIZED "{*empty*}"
|
||||
|
||||
#define FS_TRACE(...)
|
||||
#define FS_TRACE(...) // TRACE
|
||||
|
||||
namespace app {
|
||||
|
||||
namespace {
|
||||
|
||||
class FileItem;
|
||||
typedef std::map<std::string, FileItem*> FileItemMap;
|
||||
|
||||
// the root of the file-system
|
||||
FileItem* rootitem = nullptr;
|
||||
FileItemMap* fileitems_map = nullptr;
|
||||
unsigned int current_file_system_version = 0;
|
||||
|
||||
#ifdef _WIN32
|
||||
IMalloc* shl_imalloc = nullptr;
|
||||
IShellFolder* shl_idesktop = nullptr;
|
||||
#endif
|
||||
|
||||
// a position in the file-system
|
||||
class FileItem : public IFileItem {
|
||||
public:
|
||||
// TODO make all these fields private
|
||||
std::string m_keyname;
|
||||
std::string m_filename;
|
||||
std::string m_displayname;
|
||||
@ -62,7 +79,7 @@ public:
|
||||
FileItemList m_children;
|
||||
unsigned int m_version;
|
||||
bool m_removed;
|
||||
bool m_is_folder;
|
||||
mutable bool m_is_folder;
|
||||
double m_thumbnailProgress;
|
||||
os::Surface* m_thumbnail;
|
||||
#ifdef _WIN32
|
||||
@ -87,10 +104,11 @@ public:
|
||||
bool isFolder() const override;
|
||||
bool isBrowsable() const override;
|
||||
bool isHidden() const override;
|
||||
bool isExistent() const override;
|
||||
|
||||
std::string keyName() const override;
|
||||
std::string fileName() const override;
|
||||
std::string displayName() const override;
|
||||
const std::string& keyName() const override;
|
||||
const std::string& fileName() const override;
|
||||
const std::string& displayName() const override;
|
||||
|
||||
IFileItem* parent() const override;
|
||||
const FileItemList& children() override;
|
||||
@ -105,23 +123,37 @@ public:
|
||||
|
||||
os::Surface* getThumbnail() override;
|
||||
void setThumbnail(os::Surface* thumbnail) override;
|
||||
|
||||
// Calls "delete this"
|
||||
void deleteItem() {
|
||||
FileSystemModule::instance()->ItemRemoved(this);
|
||||
|
||||
if (m_parent) {
|
||||
auto& container = m_parent->m_children;
|
||||
auto it = std::find(container.begin(), container.end(), this);
|
||||
if (it != container.end())
|
||||
container.erase(it);
|
||||
}
|
||||
|
||||
auto it = fileitems_map->find(m_keyname);
|
||||
if (it != fileitems_map->end())
|
||||
fileitems_map->erase(it);
|
||||
|
||||
// Delete all children recursively
|
||||
for (auto ichild : m_children) {
|
||||
FileItem* child = static_cast<FileItem*>(ichild);
|
||||
child->m_parent = nullptr;
|
||||
child->deleteItem();
|
||||
}
|
||||
|
||||
delete this;
|
||||
}
|
||||
};
|
||||
|
||||
typedef std::map<std::string, FileItem*> FileItemMap;
|
||||
|
||||
// the root of the file-system
|
||||
static FileItem* rootitem = NULL;
|
||||
static FileItemMap* fileitems_map;
|
||||
static unsigned int current_file_system_version = 0;
|
||||
|
||||
#ifdef _WIN32
|
||||
static IMalloc* shl_imalloc = NULL;
|
||||
static IShellFolder* shl_idesktop = NULL;
|
||||
#endif
|
||||
} // anonymous namespace
|
||||
|
||||
// A more easy PIDLs interface (without using the SH* & IL* routines of W2K)
|
||||
#ifdef _WIN32
|
||||
static bool is_sfgaof_folder(SFGAOF attrib);
|
||||
static void update_by_pidl(FileItem* fileitem, SFGAOF attrib);
|
||||
static LPITEMIDLIST concat_pidl(LPITEMIDLIST pidlHead, LPITEMIDLIST pidlTail);
|
||||
static UINT get_pidl_size(LPITEMIDLIST pidl);
|
||||
@ -141,7 +173,7 @@ static unsigned int current_file_system_version = 0;
|
||||
static void put_fileitem(FileItem* fileitem);
|
||||
#endif
|
||||
|
||||
FileSystemModule* FileSystemModule::m_instance = NULL;
|
||||
FileSystemModule* FileSystemModule::m_instance = nullptr;
|
||||
|
||||
FileSystemModule::FileSystemModule()
|
||||
{
|
||||
@ -174,8 +206,7 @@ FileSystemModule::~FileSystemModule()
|
||||
{
|
||||
ASSERT(m_instance == this);
|
||||
|
||||
for (FileItemMap::iterator
|
||||
it=fileitems_map->begin(); it!=fileitems_map->end(); ++it) {
|
||||
for (auto it=fileitems_map->begin(); it!=fileitems_map->end(); ++it) {
|
||||
delete it->second;
|
||||
}
|
||||
fileitems_map->clear();
|
||||
@ -220,8 +251,8 @@ IFileItem* FileSystemModule::getRootFileItem()
|
||||
{
|
||||
// get the desktop PIDL
|
||||
LPITEMIDLIST pidl = NULL;
|
||||
|
||||
if (SHGetSpecialFolderLocation(NULL, CSIDL_DESKTOP, &pidl) != S_OK) {
|
||||
HRESULT hr = SHGetSpecialFolderLocation(NULL, CSIDL_DESKTOP, &pidl);
|
||||
if (hr != S_OK) {
|
||||
// TODO do something better
|
||||
ASSERT(false);
|
||||
exit(1);
|
||||
@ -261,6 +292,7 @@ IFileItem* FileSystemModule::getFileItemFromPath(const std::string& path)
|
||||
LPITEMIDLIST fullpidl = NULL;
|
||||
SFGAOF attrib = SFGAO_FOLDER;
|
||||
|
||||
// Default folder is desktop folder (the root item in the hierarchy)
|
||||
if (path.empty()) {
|
||||
fileitem = getRootFileItem();
|
||||
//LOG("FS: > %p (root)\n", fileitem);
|
||||
@ -280,9 +312,15 @@ IFileItem* FileSystemModule::getFileItemFromPath(const std::string& path)
|
||||
}
|
||||
#else
|
||||
{
|
||||
// The default folder is the user home folder
|
||||
if (path.empty()) {
|
||||
fileitem = get_fileitem_by_path(base::get_user_docs_folder(), true);
|
||||
}
|
||||
else {
|
||||
std::string buf = remove_backslash_if_needed(path);
|
||||
fileitem = get_fileitem_by_path(buf, true);
|
||||
}
|
||||
}
|
||||
#endif
|
||||
|
||||
//LOG("FS: get_fileitem_from_path(%s) -> %p\n", path.c_str(), fileitem);
|
||||
@ -317,21 +355,49 @@ bool FileItem::isHidden() const
|
||||
#endif
|
||||
}
|
||||
|
||||
std::string FileItem::keyName() const
|
||||
bool FileItem::isExistent() const
|
||||
{
|
||||
const std::string& fn = fileName();
|
||||
|
||||
#ifdef _WIN32
|
||||
if (!fn.empty() && fn.front() == ':') { // It's a PIDL of a special location
|
||||
FS_TRACE("FS: isExistent() %s -> PIDL exists\n", fn.c_str());
|
||||
return true;
|
||||
}
|
||||
#endif
|
||||
|
||||
bool result = false;
|
||||
|
||||
if (base::is_directory(fn)) {
|
||||
result = true;
|
||||
if (!m_is_folder)
|
||||
m_is_folder = true; // Update the "is folder" flag
|
||||
}
|
||||
else if (base::is_file(fn)) {
|
||||
result = true;
|
||||
if (m_is_folder)
|
||||
m_is_folder = false;
|
||||
}
|
||||
|
||||
FS_TRACE("FS: isExistent() %s -> %s\n", fn.c_str(), (result ? "exists": "DOESN'T EXIST"));
|
||||
return result;
|
||||
}
|
||||
|
||||
const std::string& FileItem::keyName() const
|
||||
{
|
||||
ASSERT(m_keyname != NOTINITIALIZED);
|
||||
|
||||
return m_keyname;
|
||||
}
|
||||
|
||||
std::string FileItem::fileName() const
|
||||
const std::string& FileItem::fileName() const
|
||||
{
|
||||
ASSERT(m_filename != NOTINITIALIZED);
|
||||
|
||||
return m_filename;
|
||||
}
|
||||
|
||||
std::string FileItem::displayName() const
|
||||
const std::string& FileItem::displayName() const
|
||||
{
|
||||
ASSERT(m_displayname != NOTINITIALIZED);
|
||||
|
||||
@ -486,13 +552,12 @@ const FileItemList& FileItem::children()
|
||||
for (it=m_children.begin();
|
||||
it!=m_children.end(); ) {
|
||||
child = static_cast<FileItem*>(*it);
|
||||
ASSERT(child != NULL);
|
||||
ASSERT(child);
|
||||
|
||||
if (child && child->m_removed) {
|
||||
it = m_children.erase(it);
|
||||
|
||||
fileitems_map->erase(fileitems_map->find(child->m_keyname));
|
||||
delete child;
|
||||
child->m_parent = nullptr;
|
||||
child->deleteItem();
|
||||
}
|
||||
else
|
||||
++it;
|
||||
@ -635,7 +700,7 @@ static void update_by_pidl(FileItem* fileitem, SFGAOF attrib)
|
||||
|
||||
// Get the file name
|
||||
|
||||
if (pFolder != NULL &&
|
||||
if (pFolder &&
|
||||
pFolder->GetDisplayNameOf(fileitem->m_pidl,
|
||||
SHGDN_NORMAL | SHGDN_FORPARSING,
|
||||
&strret) == S_OK) {
|
||||
@ -676,7 +741,7 @@ static void update_by_pidl(FileItem* fileitem, SFGAOF attrib)
|
||||
fileitem->m_displayname = base::get_file_name(fileitem->m_filename);
|
||||
}
|
||||
|
||||
if (pFolder != NULL && pFolder != shl_idesktop) {
|
||||
if (pFolder && pFolder != shl_idesktop) {
|
||||
pFolder->Release();
|
||||
}
|
||||
}
|
||||
@ -841,20 +906,32 @@ static std::string get_key_for_pidl(LPITEMIDLIST pidl)
|
||||
|
||||
static FileItem* get_fileitem_by_fullpidl(LPITEMIDLIST fullpidl, bool create_if_not)
|
||||
{
|
||||
FileItemMap::iterator it = fileitems_map->find(get_key_for_pidl(fullpidl));
|
||||
if (it != fileitems_map->end())
|
||||
return it->second;
|
||||
auto key = get_key_for_pidl(fullpidl);
|
||||
auto it = fileitems_map->find(key);
|
||||
if (it != fileitems_map->end()) {
|
||||
FileItem* item = it->second;
|
||||
if (item->isExistent())
|
||||
return item;
|
||||
else {
|
||||
item->deleteItem();
|
||||
return nullptr;
|
||||
}
|
||||
}
|
||||
|
||||
if (!create_if_not)
|
||||
return NULL;
|
||||
return nullptr;
|
||||
|
||||
// Check if the pidl exists
|
||||
SFGAOF attrib = SFGAO_FOLDER | SFGAO_VALIDATE;
|
||||
HRESULT hr = shl_idesktop->GetAttributesOf(1, (LPCITEMIDLIST*)&fullpidl, &attrib);
|
||||
if (hr != S_OK)
|
||||
return nullptr;
|
||||
|
||||
// new file-item
|
||||
FileItem* fileitem = new FileItem(NULL);
|
||||
FileItem* fileitem = new FileItem(nullptr);
|
||||
fileitem->m_fullpidl = clone_pidl(fullpidl);
|
||||
|
||||
SFGAOF attrib = SFGAO_FOLDER;
|
||||
HRESULT hr = shl_idesktop->GetAttributesOf(1, (LPCITEMIDLIST *)&fileitem->m_fullpidl, &attrib);
|
||||
if (hr == S_OK) {
|
||||
{
|
||||
LPITEMIDLIST parent_fullpidl = clone_pidl(fileitem->m_fullpidl);
|
||||
remove_last_pidl(parent_fullpidl);
|
||||
|
||||
@ -904,9 +981,17 @@ static FileItem* get_fileitem_by_path(const std::string& path, bool create_if_no
|
||||
if (path.empty())
|
||||
return rootitem;
|
||||
|
||||
FileItemMap::iterator it = fileitems_map->find(get_key_for_filename(path));
|
||||
if (it != fileitems_map->end())
|
||||
return it->second;
|
||||
auto key = get_key_for_filename(path);
|
||||
auto it = fileitems_map->find(key);
|
||||
if (it != fileitems_map->end()) {
|
||||
FileItem* item = it->second;
|
||||
if (item->isExistent())
|
||||
return item;
|
||||
else {
|
||||
item->deleteItem();
|
||||
return nullptr;
|
||||
}
|
||||
}
|
||||
|
||||
if (!create_if_not)
|
||||
return NULL;
|
||||
|
||||
@ -1,4 +1,5 @@
|
||||
// Aseprite
|
||||
// Copyright (C) 2019 Igara Studio S.A.
|
||||
// Copyright (C) 2001-2018 David Capello
|
||||
//
|
||||
// This program is distributed under the terms of
|
||||
@ -10,6 +11,7 @@
|
||||
|
||||
#include "base/mutex.h"
|
||||
#include "base/paths.h"
|
||||
#include "obs/signal.h"
|
||||
|
||||
#include <string>
|
||||
#include <vector>
|
||||
@ -46,6 +48,8 @@ namespace app {
|
||||
void lock() { m_mutex.lock(); }
|
||||
void unlock() { m_mutex.unlock(); }
|
||||
|
||||
obs::signal<void(IFileItem*)> ItemRemoved;
|
||||
|
||||
private:
|
||||
base::mutex m_mutex;
|
||||
};
|
||||
@ -70,9 +74,13 @@ namespace app {
|
||||
virtual bool isBrowsable() const = 0;
|
||||
virtual bool isHidden() const = 0;
|
||||
|
||||
virtual std::string keyName() const = 0;
|
||||
virtual std::string fileName() const = 0;
|
||||
virtual std::string displayName() const = 0;
|
||||
// Returns false if this item doesn't exist anymore (e.g. a file
|
||||
// or folder that was deleted from other process).
|
||||
virtual bool isExistent() const = 0;
|
||||
|
||||
virtual const std::string& keyName() const = 0;
|
||||
virtual const std::string& fileName() const = 0;
|
||||
virtual const std::string& displayName() const = 0;
|
||||
|
||||
virtual IFileItem* parent() const = 0;
|
||||
virtual const FileItemList& children() = 0;
|
||||
|
||||
@ -25,6 +25,7 @@
|
||||
#include "app/ui/skin/skin_theme.h"
|
||||
#include "app/widget_loader.h"
|
||||
#include "base/bind.h"
|
||||
#include "base/clamp.h"
|
||||
#include "base/convert_to.h"
|
||||
#include "base/fs.h"
|
||||
#include "base/paths.h"
|
||||
@ -47,6 +48,8 @@
|
||||
# define MAX_PATH 4096 // TODO this is needed for Linux, is it correct?
|
||||
#endif
|
||||
|
||||
#define FILESEL_TRACE(...) // TRACE
|
||||
|
||||
namespace app {
|
||||
|
||||
using namespace app::skin;
|
||||
@ -63,6 +66,9 @@ public:
|
||||
|
||||
bool is_null() const { return m_isNull; }
|
||||
bool is_valid() const { return !m_isNull; }
|
||||
bool exists() const {
|
||||
return (is_valid() && (*m_iterator)->isExistent());
|
||||
}
|
||||
|
||||
iterator get() {
|
||||
ASSERT(!m_isNull);
|
||||
@ -95,6 +101,46 @@ NullableIterator<FileItemList> navigation_position; // Current position in the n
|
||||
// the user. It's used only in FileSelector::Open type of dialogs.
|
||||
std::map<std::string, base::paths> preferred_open_extensions;
|
||||
|
||||
void adjust_navigation_history(IFileItem* item)
|
||||
{
|
||||
auto it = navigation_history.begin();
|
||||
const bool valid = navigation_position.is_valid();
|
||||
int pos = (valid ? int(navigation_position.get() - it): 0);
|
||||
|
||||
FILESEL_TRACE("FILESEL: Removed item '%s' detected (%p)\n",
|
||||
item->fileName().c_str(), item);
|
||||
if (valid) {
|
||||
FILESEL_TRACE("FILESEL: Old navigation pos [%d] = %s\n",
|
||||
pos, (*navigation_position.get())->fileName().c_str());
|
||||
}
|
||||
|
||||
while (true) {
|
||||
it = std::find(it, navigation_history.end(), item);
|
||||
if (it == navigation_history.end())
|
||||
break;
|
||||
|
||||
FILESEL_TRACE("FILESEL: Erase navigation pos [%d] = %s\n", pos,
|
||||
(*it)->fileName().c_str());
|
||||
|
||||
if (pos >= it-navigation_history.begin())
|
||||
--pos;
|
||||
|
||||
it = navigation_history.erase(it);
|
||||
}
|
||||
|
||||
if (valid && !navigation_history.empty()) {
|
||||
pos = base::clamp(pos, 0, (int)navigation_history.size()-1);
|
||||
navigation_position.set(navigation_history.begin() + pos);
|
||||
|
||||
FILESEL_TRACE("FILESEL: New navigation pos [%d] = %s\n",
|
||||
pos, (*navigation_position.get())->fileName().c_str());
|
||||
}
|
||||
else {
|
||||
navigation_position.reset();
|
||||
FILESEL_TRACE("FILESEL: Without new navigation pos\n");
|
||||
}
|
||||
}
|
||||
|
||||
std::string merge_paths(const base::paths& paths)
|
||||
{
|
||||
std::string k;
|
||||
@ -344,6 +390,11 @@ bool FileSelector::show(
|
||||
FileSystemModule* fs = FileSystemModule::instance();
|
||||
LockFS lock(fs);
|
||||
|
||||
// Connection used to remove items from the navigation history that
|
||||
// are not found in the file system anymore.
|
||||
obs::scoped_connection conn =
|
||||
fs->ItemRemoved.connect(&adjust_navigation_history);
|
||||
|
||||
fs->refresh();
|
||||
|
||||
// we have to find where the user should begin to browse files (start_folder)
|
||||
@ -369,7 +420,7 @@ bool FileSelector::show(
|
||||
if (!start_folder)
|
||||
start_folder = fs->getFileItemFromPath(start_folder_path);
|
||||
|
||||
TRACE("FILESEL: Start folder '%s' (%p)\n", start_folder_path.c_str(), start_folder);
|
||||
FILESEL_TRACE("FILESEL: Start folder '%s' (%p)\n", start_folder_path.c_str(), start_folder);
|
||||
|
||||
setMinSize(gfx::Size(ui::display_w()*9/10, ui::display_h()*9/10));
|
||||
remapWindow();
|
||||
@ -754,32 +805,55 @@ void FileSelector::addInNavigationHistory(IFileItem* folder)
|
||||
void FileSelector::onGoBack()
|
||||
{
|
||||
if (navigation_history.size() > 1) {
|
||||
// The default navigation position is at the end of the history
|
||||
if (navigation_position.is_null())
|
||||
navigation_position.set(navigation_history.end()-1);
|
||||
|
||||
if (navigation_position.get() != navigation_history.begin()) {
|
||||
// Go back to the first existent element
|
||||
do {
|
||||
navigation_position.set(navigation_position.get()-1);
|
||||
} while (!navigation_position.exists() &&
|
||||
navigation_position.get() != navigation_history.begin());
|
||||
|
||||
if (navigation_position.exists()) {
|
||||
m_navigationLocked = true;
|
||||
m_fileList->setCurrentFolder(*navigation_position.get());
|
||||
m_navigationLocked = false;
|
||||
}
|
||||
else {
|
||||
navigation_position.reset();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
void FileSelector::onGoForward()
|
||||
{
|
||||
if (navigation_history.size() > 1) {
|
||||
if (navigation_position.is_null())
|
||||
navigation_position.set(navigation_history.begin());
|
||||
// This should not happen, because the forward button should be
|
||||
// disabled when the navigation position is null.
|
||||
if (navigation_position.is_null()) {
|
||||
ASSERT(false);
|
||||
navigation_position.set(navigation_history.end()-1);
|
||||
}
|
||||
|
||||
if (navigation_position.get() != navigation_history.end()-1) {
|
||||
// Go forward to the first existent element
|
||||
do {
|
||||
navigation_position.set(navigation_position.get()+1);
|
||||
} while (!navigation_position.exists() &&
|
||||
navigation_position.get() != navigation_history.end()-1);
|
||||
|
||||
if (navigation_position.exists()) {
|
||||
m_navigationLocked = true;
|
||||
m_fileList->setCurrentFolder(*navigation_position.get());
|
||||
m_navigationLocked = false;
|
||||
}
|
||||
else {
|
||||
navigation_position.reset();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user