Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Fix/update default plot directory #348

Merged
merged 8 commits into from
Sep 29, 2022
Merged

Conversation

dnoishi
Copy link
Contributor

@dnoishi dnoishi commented Sep 27, 2022

No description provided.

@dnoishi dnoishi linked an issue Sep 27, 2022 that may be closed by this pull request
6 tasks
Copy link
Contributor

@ozgunozerk ozgunozerk left a comment

Choose a reason for hiding this comment

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

1- With the new changes, the check on line 262 (https://github.com/subspace/subspace-desktop/pull/348/files#diff-e879b9701cf110ae7a5636e48ba2803b2f6a8b808b3539e2937607f7a0200d11R262) won't be necessary anymore. We can simplify that check

2- Here is the bug introduced with these changes:

  • default plot path is now previous path / plots, which is a non-existent directory.
  • this directory will be created when user confirms to proceed to plotting in the SetupPlot page.
  • However, because this directory not exist whilst the user is on the SetupPlot, when user clicks the folder icon, and then cancel the dialog box back again, the backend function get_disk_stats from the utils.rs gets triggered. And crashes the application
  • the reason is, get_disk_stats is taking a directory as its parameter, and when this directory is non-existent, it crashes, naturally.

We have to call get_disk_stats after the cancel action, since the user may have selected another disk. So it's not an option to: not call get_disk_stats after cancel.

@ozgunozerk
Copy link
Contributor

My suggestion would be (for the 2nd item):

immediately create the plot directory if it is not found. We are working in the subspace-desktop directory nevertheless, so creating a folder inside that folder, I don't think we need an explicit permission from the user for that. That wouldn't make our users angry imo :)

@ozgunozerk
Copy link
Contributor

ozgunozerk commented Sep 28, 2022

Another suggestion would be, if plot directory does not exist yet, and we don't want to create it without prompting the user. Then, we can supply the subspace-desktop folder (1 level upper in this case compared to plots) to the get_disk_stats call instead. That would also solve the problem.

But this approach would require extra ifs, which would look not intuitive and make the code uglier. So if everyone is okay with the previous suggestion, my vote is on that one.

@dnoishi dnoishi requested a review from ozgunozerk September 28, 2022 10:46
@ozgunozerk
Copy link
Contributor

ozgunozerk commented Sep 28, 2022

Thanks @natachadelarosa !
I think this is good to go for the user's perspective.

There are 2 small issues left imo:
1- when the application proceed into SetupPlot we are getting this error on the console
image

2- And when the folder icon is clicked, we are getting the last error on the console
image

@isSerge did a great job cleaning all the red entries on the console, keeping it clean as it is may be a good idea :)

@dnoishi dnoishi requested a review from ozgunozerk September 28, 2022 15:17
@@ -230,6 +230,7 @@ export default defineComponent({
await this.updateDriveStats();
const path = (await tauri.path.dataDir()) + util.appName + util.PLOT_FOLDER;
this.store.setPlotPath(path);
this.createDefaultPlotDir();
Copy link
Contributor

@ozgunozerk ozgunozerk Sep 28, 2022

Choose a reason for hiding this comment

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

await?
other than that, this looks great to me, I don't have any more suggestions :)

@ozgunozerk ozgunozerk added this to the Audit Fixes milestone Sep 29, 2022
@dnoishi dnoishi merged commit fece3dd into main Sep 29, 2022
@dnoishi dnoishi deleted the fix/update-default-plot-directory branch September 29, 2022 14:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Trail of Bits - Code Quality Recommendations
2 participants