⚙ D1558 Locate Clip context menu item

benjaminnelan updated this revision to Diff 3693.May 8 2016, 11:21 AM

benjaminnelan retitled this revision from to Locate Clip context menu item.

benjaminnelan updated this object.

benjaminnelan edited the test plan for this revision. (Show Details)

benjaminnelan added a reviewer: Kdenlive.

benjaminnelan set the repository for this revision to R158 Kdenlive.

benjaminnelan added a project: Kdenlive.

Apologies for creating this again. Shortly after publishing the diff before, I realised that my code didn't account for files that weren't imported into Kdenlive, and therefore didn't have a location to open.

Not entirely familiar with Phabricator yet, so if there was a way to edit my previous diff... I completely missed it.

mardelle added a subscriber: mardelle.May 8 2016, 6:04 PM

mardelle added inline comments.

src/bin/bin.cpp
851	Please use url.adjusted(QUrl::RemoveFilename) to get the directory URL. I think you can just use the "update diff" button on top left to upload an updated diff file, not sure

Hopefully this adds this diff onto the previous diff...
Thanks for the tip with QUrl::RemoveFilename - I should have read the docs more carefully!

The changes to mltconnection are purely so I could debug, not sure why the QStringLiterals were causing problems, I didn't think they'd been changed since I last debugged.

Edit: So it's replaced it? Not sure how that works...

Sorry for the delay. Looks like your updated diff only contains changes since the previous diff.
I am not really familiar with phabricator either, but what did you upload ? Usually, to update a diff, you need to upload the updated diff against git master, not against your previous diff.
Edit: you can find instructions here
https://community.kde.org/Infrastructure/Phabricator#Workflow

Properly updated the diff. Removed all diff info that isn't related to the feature also.

Thanks mardelle!

mardelle added inline comments.May 13 2016, 6:50 PM

src/bin/bin.cpp
843	Not used, you can remove this line I think
845	Sorry I didn't spot this on my first review, but you should probably just use the ProjectClip url() method to retrieve url from clip, like: QUrl url = currentItem->url(); instead of requesting the whole xml.

Ditched uneccessary code and made a few changes to make the code more robust.
Previously, MLT files couldn't be located and placeholders (in the event of missing project footage) would display a 'Locate Clip' context item, but wouldn't actually tell the user if the footage couldn't be located.

Also opted for the clip function 'hasUrl' in order to determine whether a file is an imported one, however I discovered it didn't exist... hence my probably dodgy attempt at making up for the functionality.

Thanks for your contribution, it seems fine to me now, except that your patch seems to be in the reverse direction (from your version to git master instead of from git master to your version). Once this is fixed we can commit. Do you have a git write access?

In D1558#30359, @mardelle wrote:

Thanks for your contribution, it seems fine to me now, except that your patch seems to be in the reverse direction (from your version to git master instead of from git master to your version). Once this is fixed we can commit. Do you have a git write access?

Oh nuts, I'm not sure how I've managed to do that. Is it reverse in the sense that I haven't pulled the latest commits or is it a matter of being on the wrong branch?

I don't believe I have write access.

Oh nuts, I'm not sure how I've managed to do that. Is it reverse in the sense that I haven't pulled the latest commits or is it a matter of being on the wrong branch?

If I download your Raw Diff, I get something like:

     void slotSaveClipMarkers(const QString &id);
     void slotDuplicateClip();
-    void slotLocateClip();
     void slotDeleteEffect(const QString &id, QDomElement effect);

But it should be :

     void slotSaveClipMarkers(const QString &id);
     void slotDuplicateClip();
+    void slotLocateClip();
     void slotDeleteEffect(const QString &id, QDomElement effect);

So I guess you were in git master and created a diff against the changes in your branch instead of the other way round, something like this.
If you can get a correct patch, would be easier.

I don't believe I have write access.

Ok, I will commit for you as soon as we get a patch that applies.

Fixed my silly reverse diff issue.

Also thought I'd mention that there probably needs to be an update of the 'setVisible' and 'setEnabled' functions when clicking the 'Project' drop down in the top menu bar. Currently these only fire when you right click on items in the bin.

Which means the actions under the Project menu will reflect the last clip in the project bin that was right clicked, rather than the current item that is selected. Obviously not a huge issue, just a minor interface thing. But something I might even look into as your time is better spent on the awesome new features. :D

Apologies for updating the diff so soon after the last.

Discovered that I wasn't testing for items with urls in the slotLocateClip function.

mardelle mentioned this in R158:996e3feb0d28: Add Locate clip to open its folder in a file manager Patch by Benjamin Nelan….May 21 2016, 7:46 AM

Applied thanks. Any contribution is welcome so if you want to send more patches that's great.

This revision is now accepted and ready to land.May 21 2016, 7:49 AM

aacid closed this revision.Feb 26 2017, 11:11 PM

			Path	Packages
M			src/bin/bin.h (2 lines)
M			src/bin/bin.cpp (40 lines)
M			src/bin/projectclip.cpp (6 lines)
M			src/kdenliveui.rc (1 line)
M			src/mainwindow.cpp (5 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3693		May 8 2016, 11:16 AM	★	★
Diff 2	3723		May 9 2016, 11:13 AM	★	★
Diff 3	3807		May 13 2016, 12:09 PM	★	★
Diff 4	3898		May 20 2016, 1:30 AM	★	★
Diff 5	3911		May 20 2016, 9:10 PM	★	★
Diff 6	3913		May 20 2016, 9:47 PM	★	★

Locate Clip context menu item
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 3913

src/bin/bin.h

src/bin/bin.cpp

src/bin/projectclip.cpp

src/kdenliveui.rc

src/mainwindow.cpp

Locate Clip context menu itemClosedPublicActions