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

Set correct case for DB file path on open (fix for #7139) #7214

Merged
merged 1 commit into from
Sep 10, 2022

Conversation

mckeema
Copy link
Contributor

@mckeema mckeema commented Dec 9, 2021

Fixes #7139. On database open, if OS is Windows, find correct case for database filename by searching through database parent folder. If found, overwrite internal database file path with corrected file path so that, on database modification, database is not saved under a new name. For example, if your database is DB.kdbx and you open db.kdbx on the Windows command line, then add a new entry and save your database, DB.kdbx is maintained as the filename. Previously, DB.kdbx would be overwritten with db.kdbx on save.

Screenshots

Old behavior:

old.mp4

New behavior:

new.mp4

Testing strategy

I tested the change manually, as you can see in the video included above. I also ran make test.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

src/core/Database.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member

Does QFileInfo(path).absoluteFilePath() return the correct case?

@mckeema
Copy link
Contributor Author

mckeema commented Dec 9, 2021

Does QFileInfo(path).absoluteFilePath() return the correct case?

It does. I'm not sure why that didn't occur to me. I'll make that change in a bit.

Scratch that, I manually closed the database and tried again. For a database called TEST.kdbx opened as test.kdbx, QFileInfo(filePath).absoluteFilePath() returns test.kdbx. It must've just reopened my last used database on the first try.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.56%. Comparing base (612c109) to head (b121269).
Report is 386 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/DatabaseTabWidget.cpp 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7214   +/-   ##
========================================
  Coverage    64.56%   64.56%           
========================================
  Files          339      339           
  Lines        43855    43856    +1     
========================================
+ Hits         28312    28313    +1     
  Misses       15543    15543           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@mckeema
Copy link
Contributor Author

mckeema commented Dec 9, 2021

Regardless, the Windows build is still failing it seems. It built fine on my end and passed the local tests, so I'm not yet sure why that's happening.

Fixed. Just had to format the code.

// Get correct case for Windows filename (fixes #7139)
QFileInfo fileInfo(filePath);
QDir dir = fileInfo.dir();
QFileInfoList files = dir.entryInfoList();
Copy link
Contributor

@benjarobin benjarobin Dec 20, 2021

Choose a reason for hiding this comment

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

This could be simplified a bit (since by default entryList() matches with case insensitive)

    QStringList files = dir.entryList(QStringList{fileInfo.fileName()}, QDir::Files);
    if (files.size() > 0) {
        setFilePath(files.at(0));
    }

@droidmonkey
Copy link
Member

Can we simplify the code any further? I don't like the brute force search approach.

@benjarobin
Copy link
Contributor

@droidmonkey Well, I also do not like it... If you want to "simplify" it, there is this solution: https://docs.microsoft.com/en-us/windows/win32/memory/obtaining-a-file-name-from-a-file-handle?redirectedfrom=MSDN

@droidmonkey
Copy link
Member

You could use FindFirstFile Windows API: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfilea#examples

Then extract the filename from the data structure.

@droidmonkey droidmonkey added this to the v2.7.2 milestone Jun 19, 2022
@droidmonkey droidmonkey added bug pr: backport pending Pull request yet to be backported to a previous release labels Jun 19, 2022
* Fix keepassxreboot#7139 - when opening database files from the command line, ensure the correct case is fed to the program to prevent case changes during saves.
* Cleanup old code (checking for .json extension) from when KeePassXC app could act as a proxy.
@droidmonkey
Copy link
Member

I switched over to the native Windows function, moved the code closest to the source of input error (main.cpp), and cleaned up some old code.

@droidmonkey droidmonkey merged commit dd15db7 into keepassxreboot:develop Sep 10, 2022
@droidmonkey droidmonkey added pr: backported Pull request backported to previous release and removed pr: backport pending Pull request yet to be backported to a previous release labels Sep 11, 2022
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backported Pull request backported to previous release pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database filename case shouldn't be changed on save (after opening it from the command line)
5 participants