Fixed windows photo attachment and refactored input functions#4310
Fixed windows photo attachment and refactored input functions#4310gabriel-bolbotina wants to merge 11 commits intomasterfrom
Conversation
📦 Build Artifacts Ready
|
Withalion
left a comment
Withalion
left a comment
There was a problem hiding this comment.
Nice this will be great UX improvement. Just a few bits here and there, which I'm not sure myself
|
In testing we should also check if it fixes: |
📦 Build Artifacts Ready
|
Withalion
left a comment
Withalion
left a comment
There was a problem hiding this comment.
Multiple thinks to think about, but we are on a right path
There was a problem hiding this comment.
I would replace all QLatin1Char that you used with QStringLiteral
There was a problem hiding this comment.
I think QLatin1Char is more efficient in this case because it does not create a whole Qstring object and it is used only for a character.
Are there any benefits to changing them to QStringLiteral in the modified functions?
There was a problem hiding this comment.
After reading the documentation it doesn't look like it's that straight forward. This part talks mostly about what we are discussing. However I would like to highlight one thing in the beginning:
Generally, QString can be used everywhere and it will perform fine.
📦 Build Artifacts Ready
|
Withalion
left a comment
Withalion
left a comment
There was a problem hiding this comment.
Just some small changes to strings as we discussed
| str = QStringLiteral( "/complex/valid/Φ!l@#äme$%^&()-_=+[]{}`~;',.ext" ); | ||
| InputUtils::sanitizePath( str ); | ||
| QCOMPARE( str, QStringLiteral( "/complex/valid/Φ!l@#äme$%^&()-_=+[]{}`~;',.ext" ) ); | ||
| QCOMPARE( str, QStringLiteral( "/complex/valid/Φ!l@_äme$%^&()-_=+[]{}`~;',.ext" ) ); |
There was a problem hiding this comment.
Could you please note down somewhere that # is prohibited character now
There was a problem hiding this comment.
After reading the documentation it doesn't look like it's that straight forward. This part talks mostly about what we are discussing. However I would like to highlight one thing in the beginning:
Generally, QString can be used everywhere and it will perform fine.
| QString relativePath = prefixDir.relativeFilePath( cleanPath ); | ||
|
|
||
| // check if the path starts with ".." or is absolute (on Windows/different drives), it's not a "child" | ||
| if ( relativePath.startsWith( QLatin1String( ".." ) ) || QDir::isAbsolutePath( relativePath ) ) |
There was a problem hiding this comment.
| if ( relativePath.startsWith( QLatin1String( ".." ) ) || QDir::isAbsolutePath( relativePath ) ) | |
| if ( relativePath.startsWith( QLatin1StringView( ".." ) ) || QDir::isAbsolutePath( relativePath ) ) |
QLatin1String is a synonym for QLatin1StringView, but it's softly deprecated
| ); | ||
| if ( reservedNames.match( base.trimmed() ).hasMatch() ) | ||
| { | ||
| base = base.trimmed() + QLatin1Char( '_' ); |
There was a problem hiding this comment.
this will get changed to QChar
| // reassemble | ||
| if ( !suffix.isEmpty() ) | ||
| { | ||
| return base + QLatin1Char( '.' ) + suffix; |
There was a problem hiding this comment.
this will also get changed to QChar
| QUrl url = QUrl::fromUserInput( cleanPath ); | ||
| bool isUrl = path.startsWith( QLatin1String( "file:" ), Qt::CaseInsensitive ); |
There was a problem hiding this comment.
| QUrl url = QUrl::fromUserInput( cleanPath ); | |
| bool isUrl = path.startsWith( QLatin1String( "file:" ), Qt::CaseInsensitive ); | |
| const QUrl url = QUrl::fromUserInput( cleanPath ); | |
| const bool isUrl = path.startsWith( QStringLiteral( "file:" ), Qt::CaseInsensitive ); |
2 participants
Ready for review.
Modified the image paths in components that used them from 'file://' to 'file:///', which works on all the platforms, both Windows and Unix based.
I refactored the input functions that deal with paths to better accommodate system based differences for handling paths (e.g., partition letters on Windows). Also, I tried to use QDir and QFileInfo as much as possible.
Tested adding, viewing and saving photos on:
video-window-photo-fix.mov
Also ran the testUtils and testSketching on Windows as well.