Merge lp:~carlos-mazieri/ubuntu-filemanager-app/samba-actions-06 into lp:ubuntu-filemanager-app

Proposed by Carlos Jose Mazieri on 2015-07-18
Status: Merged
Approved by: Arto Jalkanen on 2015-08-07
Approved revision: 437
Merged at revision: 442
Proposed branch: lp:~carlos-mazieri/ubuntu-filemanager-app/samba-actions-06
Merge into: lp:ubuntu-filemanager-app
Prerequisite: lp:~carlos-mazieri/ubuntu-filemanager-app/samba-actions-05
Diff against target: 320 lines (+151/-32)
2 files modified
src/plugin/folderlistmodel/filesystemaction.cpp (+143/-31)
src/plugin/folderlistmodel/filesystemaction.h (+8/-1)
To merge this branch: bzr merge lp:~carlos-mazieri/ubuntu-filemanager-app/samba-actions-06
Reviewer Review Type Date Requested Status
Arto Jalkanen 2015-07-18 Approve on 2015-08-07
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2015-07-18
Review via email: mp+265197@code.launchpad.net

Commit message

Introduced Location on Actions, every Action will keep its "sourceLocation" and "targetLocation".

It will be possible to distinguish if an Action is performed in Local Disk only or if there is a remote Location involved.

Some DirItemInfo objects are created by respective Locations which items belong to.

Description of the change

Introduced Location on Actions.

To post a comment you must log in.
Arto Jalkanen (ajalkane) wrote :

Comment on line 35.

review: Needs Information

Answer from comment on line 35:

Yes, we can compare instances here because there will be just one instance for each type: one for DiskLocation, one for TrashLocation and another for SmbLocation. Locations are created by LocationsFactory creator class. See http://bazaar.launchpad.net/~ubuntu-filemanager-dev/ubuntu-filemanager-app/trunk/view/head:/src/plugin/folderlistmodel/locationsfactory.cpp#L46.

There is a Location::type() which can be used here, I can change that if you think that it is really necessary to make it clear. Please let me know about that.

Arto Jalkanen (ajalkane) wrote :

Thanks for the explanation, now it makes sense. I guess some clarification would be helpful there for people not so familiar with the details.

I have three suggestions and you can decide whether to do one of them:

 - Add a comment to explain it
 - Use the type()
 - Create operator overload Location::operator==() to compare the objects and there do the pointer comparison.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/plugin/folderlistmodel/filesystemaction.cpp'
2--- src/plugin/folderlistmodel/filesystemaction.cpp 2015-07-18 21:50:43 +0000
3+++ src/plugin/folderlistmodel/filesystemaction.cpp 2015-07-18 21:50:44 +0000
4@@ -128,10 +128,36 @@
5 //it is not necessary to delete
6 auxAction = 0;
7 copyFile.clear();
8-
9-}
10-
11-
12+ sourceLocation = 0;
13+ targetLocation = 0;
14+
15+}
16+
17+/*!
18+ * \brief FileSystemAction::Action::toggleLocation()
19+ *
20+ * It may be useful if there is a Undo Action to do a inverse Action
21+ */
22+void FileSystemAction::Action::toggleLocation()
23+{
24+ Location * tmp = sourceLocation;
25+ sourceLocation = targetLocation;
26+ targetLocation = tmp;
27+}
28+
29+/*!
30+ * \brief FileSystemAction::Action::matchLocations
31+ * \return true if sourceLocation is equal targetLocation
32+ */
33+bool FileSystemAction::Action::matchLocations() const
34+{
35+ return sourceLocation == targetLocation;
36+}
37+
38+bool FileSystemAction::Action::isRemote() const
39+{
40+ return sourceLocation->isRemote() || targetLocation->isRemote();
41+}
42
43 //===============================================================================================
44 FileSystemAction::ActionEntry::ActionEntry(): newName(0)
45@@ -224,15 +250,40 @@
46
47 //===============================================================================================
48 /*!
49- * \brief FileSystemAction::createAction
50+ * \brief FileSystemAction::createAction() Creates an Action struture
51 * \param type
52- * \param origBase
53+ * \param pathUrl the source URL (just the first one) to find the location
54 * \return
55 */
56-FileSystemAction::Action* FileSystemAction::createAction(ActionType type)
57+FileSystemAction::Action* FileSystemAction::createAction(ActionType type, const QString& pathUrl)
58 {
59 Action * action = new Action();
60 action->type = type;
61+
62+ //get Locations, normal case for paste/remove
63+ action->sourceLocation = m_locationsFactory->parse(pathUrl);
64+ action->targetLocation = m_locationsFactory->currentLocation();
65+ switch (type)
66+ {
67+ case ActionMoveToTrash:
68+ action->targetLocation = m_locationsFactory->getTrashLocation();
69+ break;
70+ case ActionRestoreFromTrash: // the current location must already be TrashLocation
71+ action->sourceLocation = m_locationsFactory->getTrashLocation();
72+ //TODO check the URL from trash
73+ action->targetLocation = m_locationsFactory->getDiskLocation();
74+ break;
75+ default:
76+ break;
77+ }
78+ if (action->sourceLocation == 0)
79+ {
80+ action->sourceLocation = m_locationsFactory->getDiskLocation();
81+ }
82+ if (action->targetLocation == 0)
83+ {
84+ action->targetLocation = m_locationsFactory->getDiskLocation();
85+ }
86 return action;
87 }
88
89@@ -263,11 +314,18 @@
90
91 bool FileSystemAction::populateEntry(Action* action, ActionEntry* entry)
92 {
93- DirItemInfo info(entry->itemPaths.source());
94- if (!info.exists())
95+ QScopedPointer<DirItemInfo> info(action->sourceLocation->newItemInfo(entry->itemPaths.source()));
96+ if (!info->exists())
97 {
98 emit error(QObject::tr("File or Directory does not exist"),
99- info.absoluteFilePath() + QObject::tr(" does not exist")
100+ info->absoluteFilePath() + QObject::tr(" does not exist")
101+ );
102+ return false;
103+ }
104+ if (info->needsAuthentication())
105+ {
106+ emit error(QObject::tr("Cannot access File or Directory"),
107+ info->absoluteFilePath() + QObject::tr(" it needs Authentication")
108 );
109 return false;
110 }
111@@ -283,24 +341,36 @@
112 break;
113 }
114 //this is the item being handled
115- entry->reversedOrder.append(info);
116+ entry->reversedOrder.append(*info);
117 // verify if the destination item already exists and it the destination path is in other file system
118 if (entry->type == ActionCopy ||
119 entry->type == ActionMove
120 )
121 {
122- DirItemInfo destination(entry->itemPaths.target());
123- entry->alreadyExists = destination.exists();
124- //check if it is possible to move items
125- if (entry->type == ActionMove && !moveUsingSameFileSystem(entry->itemPaths) )
126+ QScopedPointer<DirItemInfo> destination(action->targetLocation->newItemInfo(entry->itemPaths.target()));
127+ entry->alreadyExists = destination->exists();
128+ // if destination folder exists check for write permission
129+ QScopedPointer<DirItemInfo> parentDestination(action->targetLocation->newItemInfo(entry->itemPaths.targetPath()));
130+ if (parentDestination->exists() && !parentDestination->isWritable())
131+ {
132+ emit error(tr("Cannot copy/move items"),
133+ tr("no write permission on folder ") + destination->absoluteFilePath() );
134+ return false;
135+
136+ }
137+ //check if it is possible to move items,
138+ //when there is a remote Location it is necessary copy then remove
139+ if ( entry->type == ActionMove &&
140+ (action->isRemote() || !moveUsingSameFileSystem(entry->itemPaths))
141+ )
142 {
143 entry->type = ActionHardMoveCopy; // first step
144 }
145 }
146 //ActionMove will perform a rename, so no Directory expanding is necessary
147- if (entry->type != ActionMove && info.isDir() && !info.isSymLink())
148+ if (entry->type != ActionMove && info->isDir() && !info->isSymLink())
149 {
150- QDirIterator it(info.absoluteFilePath(),
151+ QDirIterator it(info->absoluteFilePath(),
152 QDir::AllEntries | QDir::System |
153 QDir::NoDotAndDotDot | QDir::Hidden,
154 QDirIterator::Subdirectories);
155@@ -501,7 +571,7 @@
156 case ActionCopy: // ActionHardMoveCopy is lso checked here
157 case ActionMove:
158 {
159- QScopedPointer <DirItemInfo> item(m_locationsFactory->currentLocation()->newItemInfo(curEntry->itemPaths.target()));
160+ QScopedPointer <DirItemInfo> item(m_curAction->targetLocation->newItemInfo(curEntry->itemPaths.target()));
161 if (!curEntry->added && !curEntry->alreadyExists)
162 {
163 curEntry->added = true;
164@@ -927,16 +997,34 @@
165 #if DEBUG_MESSAGES
166 qDebug() << Q_FUNC_INFO << paths;
167 #endif
168- Action *myAction = createAction(actionType);
169+ Action *myAction = createAction(actionType,paths.at(0));
170+ //in case of move, verify if it can be performed
171+ if (actionType == ActionMove && !canMoveItems(myAction, paths))
172+ {
173+ delete myAction;
174+ return;
175+ }
176+ //populate the action and put the action in the queue
177+ bool usingFullPath = myAction->isRemote() || DirItemInfo(paths.at(0)).isAbsolute();
178 for (int counter=0; counter < paths.count(); counter++)
179 {
180- DirItemInfo info(paths.at(counter));
181- if (!info.isAbsolute())
182- {
183- info.setFile(m_path, paths.at(counter));
184- }
185- ActionPaths pairPaths(info.absoluteFilePath());
186- pairPaths.setTargetPathOnly(m_path);
187+ ActionPaths pairPaths;
188+ //avoid creating a DirItemInfo if the Url/Path is already full
189+ //remove Locations may take longer to create DirItemInfo object
190+ if (!usingFullPath)
191+ {
192+ QScopedPointer <DirItemInfo> info (myAction->sourceLocation->newItemInfo(paths.at(counter)));
193+ if (!info->isAbsolute())
194+ {
195+ info->setFile(m_path, paths.at(counter));
196+ }
197+ pairPaths.setSource(info->absoluteFilePath());
198+ }
199+ else
200+ { //it is already full path/url
201+ pairPaths.setSource(paths.at(counter));
202+ }
203+ pairPaths.setTargetPathOnly(m_path);
204 addEntry(myAction, pairPaths);
205 }
206 queueAction(myAction);
207@@ -1155,7 +1243,7 @@
208 }
209 if (m_curAction->copyFile.target->remove())
210 {
211- QScopedPointer<DirItemInfo> item(m_locationsFactory->currentLocation()->newItemInfo(m_curAction->copyFile.targetName));
212+ QScopedPointer<DirItemInfo> item(m_curAction->targetLocation->newItemInfo(m_curAction->copyFile.targetName));
213 notifyActionOnItem(*item, ItemRemoved);
214 }
215 }
216@@ -1184,7 +1272,7 @@
217 notifyProgress();
218 if (m_curAction->copyFile.isEntryItem && m_curAction->copyFile.amountSavedToRefresh <= 0)
219 {
220- QScopedPointer <DirItemInfo> item(m_locationsFactory->currentLocation()->newItemInfo(m_curAction->copyFile.targetName));
221+ QScopedPointer <DirItemInfo> item(m_curAction->targetLocation->newItemInfo(m_curAction->copyFile.targetName));
222 m_curAction->copyFile.amountSavedToRefresh = AMOUNT_COPIED_TO_REFRESH_ITEM_INFO;
223 notifyActionOnItem(*item, ItemChanged);
224 }
225@@ -1325,7 +1413,7 @@
226 {
227 if (m_curAction->auxAction == 0)
228 { // this new action as Remove will remove all dirs
229- m_curAction->auxAction = createAction(ActionRemove);
230+ m_curAction->auxAction = createAction(ActionRemove, tempDir);
231 m_curAction->auxAction->isAux = true;
232 m_queuedActions.append(m_curAction->auxAction);
233 }
234@@ -1448,7 +1536,7 @@
235 */
236 void FileSystemAction::moveToTrash(const ActionPathList &pairPaths)
237 {
238- Action *moveAction = createAction(ActionMoveToTrash);
239+ Action *moveAction = createAction(ActionMoveToTrash, pairPaths.at(0).source());
240 for (int counter=0; counter < pairPaths.count(); ++counter)
241 {
242 addEntry(moveAction, pairPaths.at(counter));
243@@ -1464,7 +1552,7 @@
244 */
245 void FileSystemAction::restoreFromTrash(const ActionPathList &pairPaths)
246 {
247- Action *moveAction = createAction(ActionRestoreFromTrash);
248+ Action *moveAction = createAction(ActionRestoreFromTrash, pairPaths.at(0).source());
249 for (int counter=0; counter < pairPaths.count(); ++counter)
250 {
251 addEntry(moveAction, pairPaths.at(counter));
252@@ -1519,3 +1607,27 @@
253 }
254 }
255
256+
257+bool FileSystemAction::canMoveItems(Action *action, const QStringList& items)
258+{
259+ QScopedPointer<DirItemInfo> item(action->targetLocation->newItemInfo(items.at(0)));
260+ //check if moving items are being moved to the same placce
261+ if (action->matchLocations() &&
262+ action->sourceLocation->info()->absoluteFilePath() == item->absolutePath())
263+ {
264+ QString titleError = tr("Cannot move items");
265+ emit error(titleError,
266+ tr("origin and destination folders are the same"));
267+ return false;
268+ }
269+ //source items need to be removed, check for write permission
270+ if (!action->sourceLocation->info()->isWritable())
271+ {
272+ QString titleError = tr("Cannot move items");
273+ QString noWriteError = tr("no write permission on folder ");
274+ emit error(titleError, noWriteError + action->sourceLocation->info()->absoluteFilePath());
275+ return false;
276+ }
277+ //target permission is checked in populateEntry()
278+ return true;
279+}
280
281=== modified file 'src/plugin/folderlistmodel/filesystemaction.h'
282--- src/plugin/folderlistmodel/filesystemaction.h 2015-07-18 21:50:43 +0000
283+++ src/plugin/folderlistmodel/filesystemaction.h 2015-07-18 21:50:44 +0000
284@@ -195,6 +195,9 @@
285 Action();
286 ~Action();
287 void reset();
288+ void toggleLocation();
289+ bool matchLocations() const;
290+ bool isRemote() const;
291 ActionType type;
292 QList<ActionEntry*> entries;
293 int totalItems;
294@@ -208,6 +211,9 @@
295 bool isAux :1;
296 bool done :1;
297 int steps;
298+ Location * sourceLocation;
299+ Location * targetLocation;
300+
301 };
302
303 QVector<Action*> m_queuedActions; //!< work always at item 0, after finishing taking item 0 out
304@@ -223,7 +229,7 @@
305
306
307 private:
308- Action * createAction(ActionType);
309+ Action * createAction(ActionType, const QString& pathUrl);
310 void addEntry(Action* action, const ActionPaths& pairPaths);
311 bool populateEntry(Action* action, ActionEntry* entry);
312 void removeEntry(ActionEntry *);
313@@ -243,6 +249,7 @@
314 void createTrashInfoFileFromEntry(ActionEntry *entry);
315 void removeTrashInfoFileFromEntry(ActionEntry *entry);
316 void notifyActionOnItem(const DirItemInfo& item, ActionNotification action);
317+ bool canMoveItems(Action *action, const QStringList &items);
318
319 #if defined(REGRESSION_TEST_FOLDERLISTMODEL) //used in Unit/Regression tests
320 bool m_forceUsingOtherFS;

Subscribers

People subscribed via source and target branches