Re: Review Request 121447: Return inode/directory when isDir returns true (kfileitem)

2014-12-22 Thread David Faure 


 On Dec. 11, 2014, 2:27 p.m., Mark Gaiser wrote:
  src/core/kfileitem.cpp, line 255
  https://git.reviewboard.kde.org/r/121447/diff/1/?file=332652#file332652line255
 
  -- add here
  
  } else {
  // Fix for IO slaves that don't set UDS_MIME_TYPE for a folder.
  if (m_fileMode  QT_STAT_MASK) == QT_STAT_DIR) {
 m_entry.insert(KIO::UDSEntry::UDS_MIME_TYPE, inode/directory);
 m_mimeType = db.mimeTypeForName(inode/directory);
 m_bMimeTypeKnown = true;
  }
  }
  
  Not tested! Just written in comment box :)
  
  I think that's about all you'd need to fix this.
  But if this is accaptable is probably up to David to decide.
  
  I'm also not 100% sure that you catch all cases when readUDSEntry().
 
 Emmanuel Pescosta wrote:
 +1
 
 I also think that this is better way to fix it.
 Avoids code duplication and the correct mime type for folders is set a 
 early as possible, so other code can rely on it.

This suggestion would break the case where m_guessedMimeType is set, so it 
should at least that that one is empty too.

Anyhow, the orig patch is fine, since determineMimeType is only called once. 
KFileItem already takes care of caching the result.
And you can see the cache (d-m_mimeType) in currentMimeType() too, so the new 
code will also only run once.

There's no need to insert a UDS_MIME_TYPE entry. KFileItem's whole purpose in 
life is to encapsulate all these details with a proper API, so as long as it 
returns a correct mimetype everything's fine.

I do agree on one thing though: a small unittest in kfileitemtest would be nice 
:)


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121447/#review71804
---


On Dec. 11, 2014, 1:22 p.m., Àlex Fiestas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121447/
 ---
 
 (Updated Dec. 11, 2014, 1:22 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 If we know that the item is a dir, return directly the correct mimetype for 
 directories.
 
 More info of why this is needed at:
 https://git.reviewboard.kde.org/r/120909/
 
 
 Diffs
 -
 
   src/core/kfileitem.cpp 6a2cfa5 
 
 Diff: https://git.reviewboard.kde.org/r/121447/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Àlex Fiestas
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KPackage framework

2014-12-22 Thread Marco Martin 
On Saturday 20 December 2014, David Faure wrote:
  in the branch mart/kpluginmetadata there is an experiment in porting away
  from kservice, tough if i go all the way it should have a complete own
  copy of the plugin indexing (and indexing tool) since if it doesn't
  depend from kservice it should be completely independent from it
 
 Sounds like depending on kcoreaddons (if the plugin indexing can go there)
 would be better than duplicating the whole thing.

the biggest part is there, that is the KPluginMetadata constructor that takes 
a desktop file as argument.
I'm not sure about writing/reading the cache file (wether is shareable with 
the one of KService library plugins)

maybe two statics for KPluginMetadata..
QListKPluginMetadata KPluginMetadata::fromIndex(indexPath)
void KPluginMetadata::createIndex(QListKPluginMetadata, indexPath)

they would just save few lines tough, the rest is different bewteen indexing 
of libraries and packages.

One thing tough: since this wouldn't affect the API and if possible we would 
kinda need this framework in the next workspace release, could this go in, 
then moving parts afterwards?

-- 
Marco Martin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KPackage framework

2014-12-22 Thread David Faure 
On Monday 22 December 2014 10:21:21 Marco Martin wrote:
 On Saturday 20 December 2014, David Faure wrote:
   in the branch mart/kpluginmetadata there is an experiment in porting
   away
   from kservice, tough if i go all the way it should have a complete own
   copy of the plugin indexing (and indexing tool) since if it doesn't
   depend from kservice it should be completely independent from it
  
  Sounds like depending on kcoreaddons (if the plugin indexing can go there)
  would be better than duplicating the whole thing.
 
 the biggest part is there, that is the KPluginMetadata constructor that
 takes a desktop file as argument.
 I'm not sure about writing/reading the cache file (wether is shareable with
 the one of KService library plugins)
 
 maybe two statics for KPluginMetadata..
 QListKPluginMetadata KPluginMetadata::fromIndex(indexPath)
 void KPluginMetadata::createIndex(QListKPluginMetadata, indexPath)
 
 they would just save few lines tough, the rest is different bewteen indexing
 of libraries and packages.

Ah well, if it's mostly different then forget what I said.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121581: Plotter in kdeclarative

2014-12-22 Thread Marco Martin 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121581/
---

(Updated Dec. 22, 2014, 12:27 p.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

on kquickcontrolsaddons plugin in order to not depend from kxmlgui
findepoxy is local for now, before making it work under window too (and move it 
to ECM)


Repository: kdeclarative


Description
---

This is an alternative, mutually exclusive to
https://gerrit.vesnicky.cesnet.cz/r/#/c/244
and dependent from
https://git.reviewboard.kde.org/r/121575/

since the plotter component doesn't depend from libplasma, it may be useful to 
have it outside of libplasma, so any applciation that wants it may use it.
Any opinion whether this should go here or in libplasma is welcome.


Diffs (updated)
-

  src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt 786aaa5 
  src/qmlcontrols/kquickcontrolsaddons/kquickcontrolsaddonsplugin.cpp 0e2eb2f 
  src/qmlcontrols/kquickcontrolsaddons/plotter.h PRE-CREATION 
  src/qmlcontrols/kquickcontrolsaddons/plotter.cpp PRE-CREATION 
  CMakeLists.txt 233ccce 
  cmake/Findepoxy.cmake PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/121581/diff/


Testing
---


Thanks,

Marco Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121447: Return inode/directory when isDir returns true (kfileitem)

2014-12-22 Thread Mark Gaiser 


 On dec 11, 2014, 2:27 p.m., Mark Gaiser wrote:
  src/core/kfileitem.cpp, line 255
  https://git.reviewboard.kde.org/r/121447/diff/1/?file=332652#file332652line255
 
  -- add here
  
  } else {
  // Fix for IO slaves that don't set UDS_MIME_TYPE for a folder.
  if (m_fileMode  QT_STAT_MASK) == QT_STAT_DIR) {
 m_entry.insert(KIO::UDSEntry::UDS_MIME_TYPE, inode/directory);
 m_mimeType = db.mimeTypeForName(inode/directory);
 m_bMimeTypeKnown = true;
  }
  }
  
  Not tested! Just written in comment box :)
  
  I think that's about all you'd need to fix this.
  But if this is accaptable is probably up to David to decide.
  
  I'm also not 100% sure that you catch all cases when readUDSEntry().
 
 Emmanuel Pescosta wrote:
 +1
 
 I also think that this is better way to fix it.
 Avoids code duplication and the correct mime type for folders is set a 
 early as possible, so other code can rely on it.
 
 David Faure wrote:
 This suggestion would break the case where m_guessedMimeType is set, so 
 it should at least that that one is empty too.
 
 Anyhow, the orig patch is fine, since determineMimeType is only called 
 once. KFileItem already takes care of caching the result.
 And you can see the cache (d-m_mimeType) in currentMimeType() too, so 
 the new code will also only run once.
 
 There's no need to insert a UDS_MIME_TYPE entry. KFileItem's whole 
 purpose in life is to encapsulate all these details with a proper API, so as 
 long as it returns a correct mimetype everything's fine.
 
 I do agree on one thing though: a small unittest in kfileitemtest would 
 be nice :)

@David, Right, i missed that mimetype caching part which makes it a one time 
call to determineMimeType. Still, it seems better to me to move it to an 
initialization step since you nearly always need to know the mime type as early 
as possible anyway + you only need to add code in one place (right?).

I guess the } else { ... like i said before would become an if right after 
m_guessedMimeType is set:
if (m_guessedMimeType.isEmpty()  m_fileMode  QT_STAT_MASK) == QT_STAT_DIR) {
   m_mimeType = db.mimeTypeForName(inode/directory);
   m_bMimeTypeKnown = true;
}


- Mark


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121447/#review71804
---


On dec 11, 2014, 1:22 p.m., Àlex Fiestas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121447/
 ---
 
 (Updated dec 11, 2014, 1:22 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 If we know that the item is a dir, return directly the correct mimetype for 
 directories.
 
 More info of why this is needed at:
 https://git.reviewboard.kde.org/r/120909/
 
 
 Diffs
 -
 
   src/core/kfileitem.cpp 6a2cfa5 
 
 Diff: https://git.reviewboard.kde.org/r/121447/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Àlex Fiestas
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: New framework to review: KPackage

2014-12-22 Thread Marco Martin 
On Wednesday 10 December 2014, David Edmundson wrote:
 ​The binary is called kpackagetool. Given the complications we've had with
 frameworks co-installability does it make sense to call it kpackagetool5?
 
 The class name in kpackagetool/kpackagetool.cpp should probably be renamed
 
 Documentation at the top of PackageLoader should avoid saying Plasma quite
 so much

done.

 commented line at 773 of package.cpp looks concerning. I think it is just
 dead code.

yep, a signal that used to exist in plasma1, removed

 Installation command of plasmoids.knsrc are wrong (in fact they're wrong
 for current plasmapkg)
 Should kpackage even be providing this file? I think it should be with the
 plasmoid definition.

agree, shouldn't be there: removed.

 IMHO PackageJob probably shouldn't set a parent to the packagestructure.
 Otherwise if you create a PackageStructure on the stack then call
 install/uninstall it will delete the job before it's finished.

should be safe to don't have any parent since kjobs shuold delete themselves 
iirc? (changed to that)

  There's a Qt5 TODO on PackageJobThread::removeFolder

changed to just use QDir::removeRecursively()

-- 
Marco Martin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KPackage framework

2014-12-22 Thread Marco Martin 
On Monday 10 November 2014, Marco Martin wrote:
 Hi all,
 since at akademy there seemed the interest in it,
 I have been working on some classes i extracted from libplasma to be on
 their own, those related to Plasma::Package, since several applications
 have the interest of having scriptable or anyways non-binary content
 shippable over ghns.
 you can find them in the kpackage repository.

Since we are around ~2 weeks, I would like to move it, possibly to be released 
with next framework (since we would need it for Plasma (workspace) 5.2 would 
have to go in this frameworks release)

any objections?

-- 
Marco Martin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121447: Return inode/directory when isDir returns true (kfileitem)

2014-12-22 Thread Àlex Fiestas 


 On des. 11, 2014, 2:27 p.m., Mark Gaiser wrote:
  src/core/kfileitem.cpp, line 255
  https://git.reviewboard.kde.org/r/121447/diff/1/?file=332652#file332652line255
 
  -- add here
  
  } else {
  // Fix for IO slaves that don't set UDS_MIME_TYPE for a folder.
  if (m_fileMode  QT_STAT_MASK) == QT_STAT_DIR) {
 m_entry.insert(KIO::UDSEntry::UDS_MIME_TYPE, inode/directory);
 m_mimeType = db.mimeTypeForName(inode/directory);
 m_bMimeTypeKnown = true;
  }
  }
  
  Not tested! Just written in comment box :)
  
  I think that's about all you'd need to fix this.
  But if this is accaptable is probably up to David to decide.
  
  I'm also not 100% sure that you catch all cases when readUDSEntry().
 
 Emmanuel Pescosta wrote:
 +1
 
 I also think that this is better way to fix it.
 Avoids code duplication and the correct mime type for folders is set a 
 early as possible, so other code can rely on it.
 
 David Faure wrote:
 This suggestion would break the case where m_guessedMimeType is set, so 
 it should at least that that one is empty too.
 
 Anyhow, the orig patch is fine, since determineMimeType is only called 
 once. KFileItem already takes care of caching the result.
 And you can see the cache (d-m_mimeType) in currentMimeType() too, so 
 the new code will also only run once.
 
 There's no need to insert a UDS_MIME_TYPE entry. KFileItem's whole 
 purpose in life is to encapsulate all these details with a proper API, so as 
 long as it returns a correct mimetype everything's fine.
 
 I do agree on one thing though: a small unittest in kfileitemtest would 
 be nice :)
 
 Mark Gaiser wrote:
 @David, Right, i missed that mimetype caching part which makes it a one 
 time call to determineMimeType. Still, it seems better to me to move it to an 
 initialization step since you nearly always need to know the mime type as 
 early as possible anyway + you only need to add code in one place (right?).
 
 I guess the } else { ... like i said before would become an if right 
 after m_guessedMimeType is set:
 if (m_guessedMimeType.isEmpty()  m_fileMode  QT_STAT_MASK) == 
 QT_STAT_DIR) {
m_mimeType = db.mimeTypeForName(inode/directory);
m_bMimeTypeKnown = true;
 }

I will add tests and update the patch, sorry for that.


- Àlex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121447/#review71804
---


On des. 11, 2014, 1:22 p.m., Àlex Fiestas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121447/
 ---
 
 (Updated des. 11, 2014, 1:22 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 If we know that the item is a dir, return directly the correct mimetype for 
 directories.
 
 More info of why this is needed at:
 https://git.reviewboard.kde.org/r/120909/
 
 
 Diffs
 -
 
   src/core/kfileitem.cpp 6a2cfa5 
 
 Diff: https://git.reviewboard.kde.org/r/121447/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Àlex Fiestas
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KPackage framework

2014-12-22 Thread David Edmundson 
No objections from me
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121447: Return inode/directory when isDir returns true (kfileitem)

2014-12-22 Thread David Faure 


 On Dec. 11, 2014, 2:27 p.m., Mark Gaiser wrote:
  src/core/kfileitem.cpp, line 255
  https://git.reviewboard.kde.org/r/121447/diff/1/?file=332652#file332652line255
 
  -- add here
  
  } else {
  // Fix for IO slaves that don't set UDS_MIME_TYPE for a folder.
  if (m_fileMode  QT_STAT_MASK) == QT_STAT_DIR) {
 m_entry.insert(KIO::UDSEntry::UDS_MIME_TYPE, inode/directory);
 m_mimeType = db.mimeTypeForName(inode/directory);
 m_bMimeTypeKnown = true;
  }
  }
  
  Not tested! Just written in comment box :)
  
  I think that's about all you'd need to fix this.
  But if this is accaptable is probably up to David to decide.
  
  I'm also not 100% sure that you catch all cases when readUDSEntry().
 
 Emmanuel Pescosta wrote:
 +1
 
 I also think that this is better way to fix it.
 Avoids code duplication and the correct mime type for folders is set a 
 early as possible, so other code can rely on it.
 
 David Faure wrote:
 This suggestion would break the case where m_guessedMimeType is set, so 
 it should at least that that one is empty too.
 
 Anyhow, the orig patch is fine, since determineMimeType is only called 
 once. KFileItem already takes care of caching the result.
 And you can see the cache (d-m_mimeType) in currentMimeType() too, so 
 the new code will also only run once.
 
 There's no need to insert a UDS_MIME_TYPE entry. KFileItem's whole 
 purpose in life is to encapsulate all these details with a proper API, so as 
 long as it returns a correct mimetype everything's fine.
 
 I do agree on one thing though: a small unittest in kfileitemtest would 
 be nice :)
 
 Mark Gaiser wrote:
 @David, Right, i missed that mimetype caching part which makes it a one 
 time call to determineMimeType. Still, it seems better to me to move it to an 
 initialization step since you nearly always need to know the mime type as 
 early as possible anyway + you only need to add code in one place (right?).
 
 I guess the } else { ... like i said before would become an if right 
 after m_guessedMimeType is set:
 if (m_guessedMimeType.isEmpty()  m_fileMode  QT_STAT_MASK) == 
 QT_STAT_DIR) {
m_mimeType = db.mimeTypeForName(inode/directory);
m_bMimeTypeKnown = true;
 }
 
 Àlex Fiestas wrote:
 I will add tests and update the patch, sorry for that.

Mark: I don't agree with as early as possible. The best time is on demand. 
Some code using KFileItem doesn't care about mimetypes at all.

The current code for mimetypes is already on-demand, let's keep it that way.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121447/#review71804
---


On Dec. 11, 2014, 1:22 p.m., Àlex Fiestas wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121447/
 ---
 
 (Updated Dec. 11, 2014, 1:22 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 If we know that the item is a dir, return directly the correct mimetype for 
 directories.
 
 More info of why this is needed at:
 https://git.reviewboard.kde.org/r/120909/
 
 
 Diffs
 -
 
   src/core/kfileitem.cpp 6a2cfa5 
 
 Diff: https://git.reviewboard.kde.org/r/121447/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Àlex Fiestas
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

OSX/CI: kio placed files erroneously due to missing required backslash in path

2014-12-22 Thread Marko Käning 
Hi,

on OSX it looks like some app places files directly in ~/Library, as it 
misses to insert a backslash in between the standard location 
~/Library/Application Support” and the 3 files (user-places.xbel*) to be 
generated in there:
---
$ ls -l1 Library/Application\ Supportuser-places.xbel*
-rw--- 1 marko staff 245 Dec 10 03:23 Library/Application 
Supportuser-places.xbel
-rw--- 1 marko staff 512 Dec 10 03:23 Library/Application 
Supportuser-places.xbel.bak
-rw--- 1 marko staff   0 Dec 10 03:23 Library/Application 
Supportuser-places.xbel.tbcache
---

Grep-ing the whole code base to figure out which app caused this shows:
---
$ find WC/KDECI-builds/ -name *.cpp -exec grep -H user-places.xbel {} \;
WC/KDECI-builds/kbookmarks/autotests/kbookmarktest.cpp:const QString 
placesFile = datadir + /user-places.xbel;
WC/KDECI-builds/kio/src/filewidgets/kfileplacesmodel.cpp:// 
~/.local/share/user-places.xbel (according to freedesktop bookmarks spec), and
WC/KDECI-builds/kio/src/filewidgets/kfileplacesmodel.cpp:// 
user-places.xbel will be filled later). (ereslibre)
WC/KDECI-builds/kio/src/filewidgets/kfileplacessharedbookmarks.cpp:const 
QString file = datadir + user-places.xbel;
—--
where the last line reveals the culprit as being kio!

Fixed in 
http://commits.kde.org/kio/c5522b6931908d3fd8ad97555a3edf2a3e859b50

Ooops, should I have pushed this through Gerrit before committing?

Greets,
Marko
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel