Fix: [feature request] scale by file size (closes #91)#362
Fix: [feature request] scale by file size (closes #91)#362tcuong53-cell wants to merge 2 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
mybad, i will update |
tcuong53-cell
force-pushed
the
fix-bounty-issue-91
branch
from
cad1bff to
b32b4fa
Compare
|
updated, please check, tks |
mschilli87
left a comment
mschilli87
left a comment
There was a problem hiding this comment.
I did not try this branch and didn't look too deep into the actual implementation of the added functionality. So I cannot comment on whether or not this actually resolved the FR. However, I noticed a lot of seemingly unrelated changes that I don't see why they would be needed, how the improve the code, and what is their relation to what this PR is trying to achieve. Maybe this does contain a good implementation of the feature. But it definitely contains more than that. And maybe some of those changes are useful for some reasons as well. But even then, IMHO they should be separated into different commits and PRs and come with some explanation as to why they should be merged. I'd suggest to mark this WIP, use it as a base to discuss if the way this adds the feature is what the project wants and then refactor this into a clean PR that adds the feature with the minimum required changes only. I would not support merging this as-is.
| time_t timestamp; | ||
| if (parts.size() < 4) return false; | ||
|
|
||
| // Allow timestamp to be a string |
| if (parts.size() < 4) return false; | ||
|
|
||
| // Allow timestamp to be a string | ||
| if(entries[0].size() > 1 && entries[0].find("-", 1) != std::string::npos) { |
|
|
||
| if(written < 0 || written >= 2048) { | ||
| int restore_rc = chdir(cwd_buff); | ||
| (void)restore_rc; |
There was a problem hiding this comment.
What's the point of these additions?
|
|
||
| //change back to original directory | ||
| chdir(cwd_buff); | ||
| int chdir_rc = chdir(cwd_buff); |
There was a problem hiding this comment.
Why do you think this needs to be changed?
|
|
||
| RCommitLog::RCommitLog(const std::string& logfile, int firstChar) { | ||
|
|
||
| logfile_path = logfile; |
There was a problem hiding this comment.
What's the purpose of this new variable?
|
|
||
| // this->parent_radius = std::max(1.0, parent_circ / PI); | ||
| this->parent_radius = std::max(1.0f, (float) sqrt(total_file_area) * gGourceDirPadding); | ||
| this->parent_radius = std::max(1.0f, (float) sqrt(total_file_area)) * gGourceSettings.dir_spacing; |
| setGraphic(gGourceSettings.file_graphic); | ||
| } | ||
|
|
||
| // Only apply spiral layout forces when NOT using physics-based file sizing |
There was a problem hiding this comment.
I think it would be cleaner if this could be handled via an early return rather than another level of nesting.
|
|
||
| float name_alpha = selected ? 1.0 : alpha; | ||
|
|
||
| std::string label = gGourceSettings.file_extensions ? ext : name; |
There was a problem hiding this comment.
How is this change related to the scaling?
|
|
||
| std::string name; | ||
| float namewidth; | ||
| public: |
|
|
||
| LIBS += -lmingw32 -lSDL2main -lSDL2.dll | ||
| LIBS += -lSDL2_image.dll -lfreetype.dll -lpcre2-8.dll -lpng.dll -lglew32.dll -lboost_system-mt -lboost_filesystem-mt -lopengl32 -lglu32 | ||
| LIBS += -lSDL2_image.dll -lfreetype.dll -lpcre2-8.dll -lpng.dll -lglew32.dll -lboost_filesystem-mt -lopengl32 -lglu32 |
There was a problem hiding this comment.
How can adding an optional(!) feature result in dropping a dependency?
|
Note that I just commented on a few example issue I had skimming the diff. Even if all those were addressed, I'd need another round of review before considering supporting this PR. |
3 participants
Fix [feature request] scale by file size. Tested locally and works fine. Closes #91