BUG: 385367
Changed default save path to ~/Videos, which seems like a more humane starting location than /var/tmp.
Details
Details
- Reviewers
lesliezhai ltoscano - Group Reviewers
KDE Applications - Commits
- R467:087d694664ad: Change default save path to ~/Videos
Can't test; don't have any machines with optical drives. Would appreciate some rudimentary testing from anyone with appropriate hardware.
Diff Detail
Diff Detail
- Repository
- R467 K3b
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
The directory can be localized.
Use QStandardPaths and look for QStandardPaths::MoviesLocation; see how the "global paths KCM" in Plasma does it:
https://commits.kde.org/plasma-desktop?path=kcms/desktoppaths/globalpaths.cpp
Comment Actions
Please also note that the directory returned by QStandardPaths::writableLocation may not exist. This is a latent error also with /var/tmp, and it was working with the original value of "/tmp" before the change of https://commits.kde.org/k3b/3125c7cda1e54c1c425189a44303dc360038d777
Also KoZipStore.cpp should be fixed (and ported to QTemporaryFile, maybe in a separate change.
Side note: that patch was merged without a useful commit message and it's unclear its origin, but that's another story.
Comment Actions
This may be a little beyond me. The coding style is a little weird here; it's all done in the header file. What's the right place to put the path-checking logic?
Comment Actions
When you use that variable (exported through getVideoCdDestination) ensure that the directory exists.
Comment Actions
Hi Nathaniel,
Sorry for my late response!
I didn't receive any notification emails about this code review, but my email address is lesliezhai@llvm.org.cn please commit the patch if Luigi gave LGTM :)
The coding style is a little weird here; it's all done in the header file. What's the right place to put the path-checking logic?
It is able to put it in the *constructor* of VideoCdRippingOptions
or
just following the C++11 style, still put
m_videocddestination = XXX; // with your logic
in the header file.
Hi Luigi,
and it was working with the original value of "/tmp" before the change of https://commits.kde.org/k3b/3125c7cda1e54c1c425189a44303dc360038d777
sorry, I just want to save this patch for Fedora packagers.
Regards,
Leslie Zhai
Comment Actions
Move logic into constructor and handle the case where the user has deleted their Videos folder or made it read-only
Comment Actions
Hi Nathaniel,
I must emphasize that this is untested, as I have no CD drive.
It is able to use CDEmu to simulate CD driver.
Regards,
Leslie Zhai
Comment Actions
| src/rip/k3bvideocdrippingoptions.h | ||
|---|---|---|
| 29–30 | The old init style could be deleted. | |
| 38 | // To save variable videoPath m_videocddestination = QStandardPaths::writableLocation(QStandardPaths::MoviesLocation); | |
| src/rip/k3bvideocdrippingoptions.h | ||
|---|---|---|
| 38 | m_videocddestination = QStandardPaths::writableLocation(QStandardPaths::MoviesLocation);
QFileInfo checkPath(m_videocddestination);
if (!checkPath.exists())
m_videocddestination = QStandardPaths::writableLocation(QStandardPaths::HomeLocation); // but user has not deleted their $HOME folder. | |
| src/rip/k3bvideocdrippingoptions.h | ||
|---|---|---|
| 38 | I don't think it's possible for a user to delete their own home folder, which is why that's the fallback path. | |