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.

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 top directory in file browser #1393

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

pford
Copy link
Collaborator

@pford pford commented Dec 6, 2024

Description

  • What is implemented or fixed? Mention the linked issue(s), if any.
    Fixes [file browser] "back to starting directory" button does not work #1394

  • How does this PR solve the issue? Give a brief summary.
    casacore::Path cannot resolve "/" or "/." to absolute path due to parsing error. Instead, use casacore::Path::expandedName() which is used for casacore::File::exists() and casacore::File::isReadable().

  • Are there any companion PRs (frontend, protobuf)?
    No

  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    Open file browser and click the top directory of breadcrumbs, which is the root directory. A file list should appear instead of an error.

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / corresponding fix added / new e2e test created
  • ICD test passing / corresponding fix added / new ICD test created
  • protobuf updated to the latest dev commit / no protobuf update needed
  • protobuf version bumped / protobuf version not bumped
  • added reviewers and assignee
  • GitHub Project estimate added

@pford
Copy link
Collaborator Author

pford commented Dec 6, 2024

@confluence I started to refactor the code using std::filesystem to resolve a file path and check its type (directory or file), but it became more code maintenance than bug fix and would change many files (look for "exists" in the code!). For this issue, I only fixed the bug, and will refactor in another PR.

@pford
Copy link
Collaborator Author

pford commented Dec 6, 2024

I suppose I should also add a test for this bug fix.

@pford
Copy link
Collaborator Author

pford commented Dec 6, 2024

I suppose I should also add a test for this bug fix.

Added FileListTest for default top level folder "/", expect success=true, 0 files, >0 subdirectories.

Copy link

github-actions bot commented Dec 6, 2024

Code Coverage

Package Line Rate Health
src.Cache 72%
src.DataStream 44%
src.FileList 67%
src.Frame 36%
src.HttpServer 42%
src.ImageData 28%
src.ImageFitter 83%
src.ImageGenerators 43%
src.ImageStats 75%
src.Logger 37%
src.Main 52%
src.Region 69%
src.Session 4%
src.Table 52%
src.ThreadingManager 67%
src.Timer 85%
src.Util 38%
Summary 46% (8643 / 18965)

Copy link
Contributor

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

Looks good. The issue is gone. No other regression from e2e.

@kswang1029 kswang1029 added the awaiting code review For pull requests that require code review label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting code review For pull requests that require code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[file browser] "back to starting directory" button does not work
3 participants