Conversation
There was a problem hiding this comment.
What are positive effects of this change? I'd prefer to keep internal stuff UI framework agnostic where possible. So std::fs, if refactoring this at all. Currently all path handling is done in ascii char*, so glib functions perform well here w/o extra load and conversions.
Note: entries loading order must be checked for changes if refactoring this.
libs/os/dir.cpp
Outdated
| const char* Directory::read_and_increment(){ | ||
| if(this->dir) { | ||
| QDir* dir_instance = reinterpret_cast<QDir*>(this->dir); | ||
| QStringList entryList = dir_instance->entryList(QDir::Dirs | QDir::Files | QDir::NoDotAndDotDot); |
There was a problem hiding this comment.
This is srs perf penalty. E.g. with 1k entries in a browsed folder level at least 1M allocations will occur.
libs/os/dir.cpp
Outdated
| QString file_path = entryList.at(this->entry_idx); | ||
| QString file_name = QFileInfo(file_path).fileName(); | ||
| this->entry_idx++; | ||
| return file_name.toStdString().c_str(); |
There was a problem hiding this comment.
.toLatin1() is more correct for this sort of conversions.
And we are returning pointer to temp local variable here ⚰️
There was a problem hiding this comment.
The reason why glib was removed is because qt provides directory management therefore glib seems to be redundant.
There was a problem hiding this comment.
I would love to know your take on this, this project already heavily uses qt why not use qt core for internal stuff as well.
There was a problem hiding this comment.
Qt is used almost exclusively for UI.
Ic no positive effects of this change. It's not even removing glib dependency, which is used in other places.
While it adds dependency on big juicy framework, which may be unwelcome in case of some potential porting. Imagine doing Gtk->Qt port if not just UI was dependent on Gtk.
Also performance and behavior compatibility of this change are questionable.
| this->dir = new QDir(name); | ||
| QStringList entryList = this->dir->entryList(QDir::Dirs | QDir::Files | QDir::NoDotAndDotDot); | ||
| for(int i = 0; i < entryList.size(); i++) { | ||
| QString file_path = entryList.at(i); |
There was a problem hiding this comment.
This is unnecessary copy. QString is using hashed pool, but it's not free still. Normal practice is taking reference.
| for(int i = 0; i < entryList.size(); i++) { | ||
| QString file_path = entryList.at(i); | ||
| QString file_name = QFileInfo(file_path).fileName(); | ||
| this->entries.push_back(file_name.toLatin1().data()); |
There was a problem hiding this comment.
You store pointer to temp string here. Do you even test? This is straightforward crash.
Garux
force-pushed
the
master
branch
16 times, most recently
from
6419b55 to
db756dc
Compare
2 participants
No description provided.