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

Allow special characters in bottle names #3140

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Conversation

cybin
Copy link
Contributor

@cybin cybin commented Oct 29, 2023

  • disabled Pango markup for bottle names in dialogs
  • use bottle names md5 hash for directory creation

Description

Currently, bottle names in the UI are treated as Pango markup. This causes problems when using special characters like '<'. To fix this, the sha256 hash of a bottles name is used to create the directory. This shouldn't be a problem as the bottles name for UI is taken from bottles.yml. So, the user will be able to use those characters now. One downside is that the directory for a bottle is not named like it is shown in the UI. On the other hand, Bottles is a manager for bottles and as a user it shouldn't require me to work manually within the filesystem.

These changes does not affect already existing bottles, as far as I could test it. It just works for newly created ones. Well, this is a kind of brute force attempt and might be tested by others as I'm sure I didn't seen all possible dialogs using a bottles name.

Fixes #(issue)
#3106

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.

  • Open 'New Bottle' dialog
  • Name the bottle 'This is a nasty < Test with {}[]/'
  • Click create.

@xioren
Copy link
Contributor

xioren commented Nov 5, 2023

Minor nitpick...your code comment says hash with MD5 and yet you are hashing with SHA256 (which will produce quite long filenames/paths).

@cybin
Copy link
Contributor Author

cybin commented Nov 6, 2023

Minor nitpick...your code comment says hash with MD5 and yet you are hashing with SHA256 (which will produce quite long filenames/paths).

argh... initially I used MD5, but it did not pass the quality checks, due to weakness. Yes, I should have removed the comment. I will push an update when I got the time to do that.

The point to use a hash is that using <, ', or the like in paths is a pain. The usual approach is to simply replace them with an underscore or something. But this will lead to '<<' and '>>' be the same '__'. So, as I mentioned before, there shouldn't be need for a 'normal' user to work directly in these directories. And everyone familar to the shell to do so shouldn't have a real problem working with hashes. Well, this is my humble opinion, of course.

@orowith2os
Copy link
Contributor

Would disabling the markup here not be enough? Why do we need to also change how directories are created? The less disconnection we have between the filesystem and the UI, the better.

@cybin
Copy link
Contributor Author

cybin commented Nov 17, 2023

Would disabling the markup here not be enough? Why do we need to also change how directories are created? The less disconnection we have between the filesystem and the UI, the better.

It might be enough to simply disable the markup. There is no need to change the way you create directories, if you don't like.

@xioren
Copy link
Contributor

xioren commented Nov 18, 2023

In this case I don't think it is necessary since we already use pathvalidate to sanitize filenames/paths; otherwise I would support the change.

Copy link

fab-sonarqube bot commented Nov 19, 2023

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@orowith2os
Copy link
Contributor

I can support this MR without hashing.

Copy link
Contributor

Pylint result on modfied files:

@orowith2os orowith2os merged commit 8eefcc5 into bottlesdevs:main Nov 27, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants