BUG: 408956
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
ffmpegthumbnailer/moviedecoder.cpp | ||
---|---|---|
253 | If this variable isn't needed, just remove this line. |
Since I never used phabricator before, who needs to make the changes to the patch? Me, the submitter of the patch, or the one who finally integrates the patch?
ffmpegthumbnailer.cpp | ||
---|---|---|
51 | A quick glance at the source code of taglib revealed that in isValid() are 2 checks are done
I have directories with more than 50 MP4s all including covers and it is reasonable fast. Defintely faster than extracting the same amount of still frames out of the video. |
Thanks for the patch. Please read https://community.kde.org/Policies/Commit_Policy about how to write a good commit message.
CMakeLists.txt | ||
---|---|---|
6–21 | Please add a cmake/FindTaglib.cmake file (you can re-use https://cgit.kde.org/kio-extras.git/tree/cmake/FindTaglib.cmake) and then use find_package(Taglib) instead of requiring pkg-config. | |
ffmpegthumbnailer/filmstripfilter.h | ||
29 | Is this change related? | |
ffmpegthumbnailer/moviedecoder.cpp | ||
253–254 | Is this change related? If not, we should do it in another commit. |
@elvisangelaccio maybe we should move FindTaglib.cmake into ECM. Quite a few projects seem to have their own copies of it.
Copied https://cgit.kde.org/kio-extras.git/tree/cmake/FindTaglib.cmake and used it to get rid of pkgconfig-depency
We should probably just do this now and then remove the redundant copies of FindTagLib later.
cmake/FindTaglib.cmake | ||
---|---|---|
2 | this is present in extra-cmake-modules/attic/modules/FindTaglib.cmake I guess you can reuse this file, instead of duplicating it here. |
cmake/FindTaglib.cmake | ||
---|---|---|
2 | Nope, that's a legacy file that should not be used anymore. |
Please either copy the FindTaglib.cmake from ECM master or require KF5 >= 5.72 and don't copy FindTaglib.cmake here.
ffmpegthumbnailer.cpp | ||
---|---|---|
50 | Please use toLocal8Bit() + data() instead: https://wiki.qt.io/Technical_FAQ#How_can_I_convert_a_QString_to_char.2A_and_vice_versa.3F Also, prefer a descriptive variable name. |
- copied and replaced FindTaglib.cmake from KDE/extra-cmake-modules/master/find-modules/
- using`toLocal8Bit() + data()` to get C-string of path
You uploaded a wrong version of the patch. Please do a git rebase master and make sure you only have one commit on top of origin/master.
After struggling with git (as usual since git rebase master did nothing) I came up with that diff.
I did:
- git fetch
- git diff origin/master
I hope this diff is okay now.
This is expected, if you have an old master branch / the same as when you started your branch.
You can rebase only on branches that are not your parent.
I did:
- git fetch
- git diff origin/master
This does not edit your branch.
Once you have fetched changes on your local master, then you can git rebase master.
So run git rebase master now, that you ran git fetch. If it does nothing ensure you fetched the master branch git fetch master.
If you are working locally on your master branch, i would suggest you to checkout a new branch where you are git checkout -b diff-D23168, then reset git reset --hard master origin/master to reset your local master branch to the origin/master branch.
Then you can rebase your branch diff-D23168 on master. git checkout diff-D23168 && git rebase master.
If that's not clear I can recommend you the pro-git book reference on rebasing https://git-scm.com/book/en/v2/Git-Branching-Rebasing
How do I switch to the right repo?
git remote -v
origin http://anongit.kde.org/ffmpegthumbs (fetch)
origin http://anongit.kde.org/ffmpegthumbs (push)
I've created a branch, changed back to master, did fetchand pull, switched back to my branch and did rebase master. Nothing happened.
I have absolutely no plan what you want me to do, except I have the wrong repo and you want me to commit somewhere else. Up to now I used the web interface to submit my patches.
git reset --hard master origin/master
fatal: Cannot do hard reset with paths.
git remote set-url origin https://invent.kde.org/multimedia/ffmpegthumbs.git
git help <command> can help you, and you can learn this from https://git-scm.com/book/en/
I've created a branch, changed back to master, did fetchand pull, switched back to my branch and did rebase master. Nothing happened.
I have absolutely no plan what you want me to do, except I have the wrong repo and you want me to commit somewhere else.
Up to now I used the web interface to submit my patches.
Didn't know that. I am not familiar with the web interface but I guess that works similarly to the arc command line utility.
By the way, while we can finish this here, the project has moved to a gitlab instance at https://invent.kde.org/multimedia/ffmpegthumbs
The simplest way then would perhaps to redo you your changes starting from current master and open a merge request in https://invent.kde.org/multimedia/ffmpegthumbs.
You might be more familiar with github/gitlab workflow.
I added my ssh-key to gitlab, set the upstream urls:
origin git@invent.kde.org:multimedia/ffmpegthumbs.git (fetch)
origin git@invent.kde.org:multimedia/ffmpegthumbs.git (push)
Than I follwed the instructions here: https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_requests.html#new-merge-request-from-your-local-environment
git push origin diff-D23168
remote: remote: ======================================================================== remote: remote: You are not allowed to push code to this project. remote: remote: ======================================================================== remote: fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists.
Really, there have been easier ways to submit a patch. Initially, it was a feature request with code provided and now I struggle months to get it into KDE. :-/
You need to fork the repository in gitlab, before opening the merge request.
Refer to our own instructions https://community.kde.org/Infrastructure/GitLab#Fork_the_project
We also have a tool https://invent.kde.org/sdk/git-lab to simplify the merge request creation/process (it is similar to arc but way easier to install, to use and to contribute to).
Really, there have been easier ways to submit a patch. Initially, it was a feature request with code provided and now I struggle months to get it into KDE. :-/
IMHO Gitlab is improving things, but since things are transitioning, this adds some friction we are seeing here.
Contributing is not a simple thing : with reviewer time varying and limited and contributors experience varying, this adds some friction. The first patch is usually the hardest.
As long as you don't have to complicate things, things are mostly smooth : here you need to rebase a patch that you opened from the web ui which phabricator does not help with.
For instance git-lab offers in Web Ui a rebase button.
Hold on a little more ;)
Did a merge request.
https://invent.kde.org/multimedia/ffmpegthumbs/-/merge_requests/2
I'm willing to follow all requirements, but if it gets a technical chaos and not a chaos in my contribution, I get annoyed.
I hope the merge request is okay so far. It is ONE commit.