D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  You're completely right in the sense that for a batch rename files will be in 
the same folder. However, rename really is just a move operation. The current 
BatchRenameJob has a hardcoded logic about placeholders and extensions which 
make it unusable outside of its current scope.
  
  The proposed BatchMoveJob does exactly that: Move these files from here to 
there, no questions asked and without any manipulation of the file names.
  
  In the context of this job it Wil be used to rename files, but as a patch it 
is completely independent of this. Applications such as Dolphin or Krusader for 
example, can also use it to do their batch moves instead of implementing it 
themselves with multiple subjobs etc.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  I honestly think the GUI is mostly ready, at least what I wanted to achieve 
with it: Something that is advanced enough but not over the top with options as 
there's already tools for that. Will check at some point to finish the items 
left from the review here. What this needs to be fully implemented is support 
from KIO for a "proper" batch move operation. I've submitted a patch for that 
at D27760  but am still waiting for a 
review.
  
  I'm completely open to new ideas though.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D27760: WIP | Added BatchMoveJob

[Emirald Mateli]

emateli added inline comments.

INLINE COMMENTS

> batchmovejobtest.cpp:89
> +KIO::Job *job = KIO::batchMove(items);
> +
> KIO::FileUndoManager::self()->recordJob(KIO::FileUndoManager::CommandType::BatchMove,
>  QList{}, QUrl(), job);
> +job->setUiDelegate(nullptr);

@dfaure How do I correctly register this job so I can undo it in one operation 
(In the undo call a few lines below)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27760

To: emateli, dfaure, ngraham
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27760: WIP | Added BatchMoveJob

[Emirald Mateli]

emateli added a comment.


  In that it supports moving them to different directories but as well as 
moving them with a different name. This operation is to allow also batch 
renaming of items. See this patch for more details: D14631 

  
  eg: BatchMove {`a.txt -> Sample.txt`, `b.md -> new/Readme.md`}.  In the 
context of the mentioned patch the directory wouldn't change of course, but 
this is also a more generic move job. Move these files from here to somewhere 
else in a single job (also so that a single undo reverts everything back to the 
original place).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27760

To: emateli, dfaure, ngraham
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27760: WIP | Added BatchMoveJob

[Emirald Mateli]

emateli retitled this revision from "Added BatchMoveJob" to "WIP | Added 
BatchMoveJob".
emateli edited the summary of this revision.
emateli added reviewers: dfaure, ngraham.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27760

To: emateli, dfaure, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27760: Added BatchMoveJob

[Emirald Mateli]

emateli created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
emateli requested review of this revision.

REPOSITORY
  R241 KIO

BRANCH
  batchmove

REVISION DETAIL
  https://phabricator.kde.org/D27760

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchmovejobtest.cpp
  src/core/CMakeLists.txt
  src/core/batchmovejob.cpp
  src/core/batchmovejob.h
  src/widgets/fileundomanager.h

To: emateli
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  Well, this patch is more or less complete but it doesn't make sense to be 
deployed without batch operations from KIO as it would spawn a job for each 
file to be renamed.
  
  Like I stated in my last comment, I tried to give adding batch renaming a go 
but it felt like I needed some time to familiarize myself with kio internals 
first. So to save everyone time, I was hoping someone who has worked KIO more 
extensively is willing to implement the batch rename stuff since he'll be a lot 
faster than I. Once that is merged we can resume work here.
  
  That said I'll be giving the batch stuff another try.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  I spent some time with modifying CopyJob but didn't make much progress and 
it's too deep into KIO internals - a lot one has to familiarize himself with. 
Maybe it's best someone who already did work on KIO to add the batch stuff. 
Once that is done and merged we can resume over here.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  In D14631#499038 , @dfaure wrote:
  
  > KIO::move (implemented by CopyJob) can move N files to a single destination 
directory, but they get the same name at that dest.
  >  KIO::moveAs (implemented by CopyJob too) can move/rename a single file to 
a specific filename at destination.
  >
  > What you're suggesting is a new job that can move N files to a destination 
directory (to make this generic it doesn't have to be in-place renames, 
right?), but you're providing the filename at destination for each file, right?
  
  
  More or less but they don't have to be in the same directory. Think of it as 
a sequence of `KIO::moveAs` operations. Any N files can be moved anywhere.
  
  > I could imagine a KIO::moveAs that takes two QList and then this 
information is fetched from there rather than using m_dest.
  >  In fact, if the existing moveAs() method is ported to call the two-QLists 
one, that will mean less special casing in the code (which wouldn't use m_dest 
anymore in slotResultStating, when m_asMethod).
  
  The two list version could work, but I was thinking of one 
`QList` that contains src and dest names. Looks less error prone 
IMO. Also yes, moveAs can be implemented as a special case of this with just 
one item to move. The option of adapting move to accept a list of dest also 
involves modifying CopyJobPrivate's dest which will lead to a larger 
refactoring needed than the proposal below, right?
  
  ---
  
  I was thinking of implementing this as a new subclass of Job where it will 
create the new Job and add a subjob for each of the files to be moved. In the 
same fashion that the current `BatchRenameJob` works. Otherwise if overloading 
will not break binary compatibility then this can be an overloaded 
`KIO::moveAs(QList)` which again does the same thing, a series of 
the current `moveAs`. It also has to remain as a single job so that it can be 
undone in one go instead of undoing for each item that was renamed.
  
  e.g:
  
auto items = {
KioRenameItem{QUrl("~/a.doc"), QUrl("~/Documents/a.doc")},
KioRenameItem{QUrl("~/dir/file"), QUrl("~/Documents/file-2019")},
};
  
  If this looks OK then I'll just get started with this and continue this part 
of the work in another patch.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  Before continuing work I'd like to hear some opinions on this.
  
  1. From @dfaure or other people at frameworks: How do we handle batch rejame 
jobs. Should I create a new patch to submit a basic batch move job that simply 
moves files around without any additional processing, that way it can also 
serve as a generic batch move job(I have written in previous comments more on 
this how I think it should look like). As I don't think going back to doing 
many single move jobs is the best of ideas. Also this patch is getting way too 
big and difficult to review.
  2. From everyone else (also pinging @ngraham) since we're doing this and the 
code sans the KIO batch stuff is almost done, are there any additional features 
we might want to take into consideration? E.g: File metadata?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  I'll need to rewrite the BatchRenameJob part which does the batch processing 
without breaking binary compatibility. Been unusually busy lately between job 
and uni but I'll start working on this sooner than later.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D18563: Don't create directory tree when a new folder has a '/' in the name

[Emirald Mateli]

emateli added a comment.


  Agree, something similar is being proposed over at D18384 
. These dialogs are getting out of hand.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18563

To: shubham, ngraham, #frameworks, #dolphin, dfaure, elvisangelaccio, pino
Cc: emateli, cfeck, acrouthamel, markg, ndavis, dfaure, elvisangelaccio, pino, 
kde-frameworks-devel, michaelh, ngraham, bruns

D18384: Allow creating directory named '~' and throw a warning before creating it.

[Emirald Mateli]

emateli added a comment.


  I am of the opinion that such dialogs are not very user friendly and honestly 
very much against them. Not commenting on the usefulness of this particular 
warning, but dolphin has quite a few of these kind of dialogs with the same 
process: `Input something -> Get error -> Restart`. Feedback should instead be 
given inplace.
  
  A new dialog built especially for Dolphin's new file/folder/X should take 
over getting the input from the user and prevent continuing or warn the user 
about his actions:
  
  - User leaves empty input? "Name cannot be empty".
  - User types existing folder name? Show him an error message on the spot
  - User types "~" as part of the file name? Show this warning
  - User types names with `/`, warn that a path will be created instead of just 
a single directory.
  - User types a name that starts with a dot? Show him the current warning 
about hidden files etc.
  
  I am strongly of the opinion that such dialog is vastly more user friendly to 
what we currently have and want to implement. Pinging @ngraham since he's 
particularly concerned with UX/Usability these days. Looking forward to 
everyone's thoughts.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18384

To: shubham, ngraham
Cc: emateli, pino, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli planned changes to this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a subscriber: chinmoyr.
emateli added a comment.


  Pinging @chinmoyr as the original author of the BatchRenameJob class.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 47694.
emateli added a comment.


  - convert to namespace from class
  - do not show first captured group
  - remove help button
  - Use d-ptr pattern
  - remove show/hide methods and autoshow dialog
  - update doc
  - KFileItemList -> QList; KFileItem usage was unnecessary
  - update doc
  - WIP Change BatchRenameJobPrivate class
  
  This is still a WIP mostly with the use of d-ptr pattern for the exported 
header and the conversion of BatchRenameJob from a class with a very specific 
purpose to a general-purpose batch rename operation.
  
  Looking for comments especially for the repurposing of the BatchRenameJob 
class(currently used only by Doplhin) and other thoughts in general as well.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=45438=47694

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/core/batchrenamejob.cpp
  src/core/batchrenamejob.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp
  tests/previewtest.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Emirald Mateli]

emateli added a comment.


  FWIW: The patch over at D11304  changes 
the behavior to select the currently existing folder instead of doing nothing.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17617

To: ngraham, #dolphin, elvisangelaccio
Cc: emateli, elvisangelaccio, Codezela, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Emirald Mateli]

emateli added a comment.


  IMO it would be great if this would just rename inline rather than opening a 
dialog.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: emateli, elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, 
michaelh, ngraham, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli marked 3 inline comments as done.
emateli added a comment.


  There's still some work to do with the actual BatchRenameClass now and the 
d-pointer pattern. I'll update this in a few days.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D16694: Improve visibility for Konsole icon when using Breeze Dark

[Emirald Mateli]

emateli added a comment.


  @anishgiri What is the advantage of using this outlined version of the 
Konsole icon(which we haven't seen in a light scheme yet) compared to using one 
of the monochrome icons already available?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D16694

To: anishgiri, ngraham, #vdg
Cc: emateli, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16694: Improve visibility for Konsole icon when using Breeze Dark

[Emirald Mateli]

emateli added a comment.


  Agree with @ndavis with respect to the icon. I would be in favor of using 
something similar (if not identical) to `dialog-scripts`. It's monochrome and 
works with dark and light backgrounds.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D16694

To: anishgiri, ngraham, #vdg
Cc: emateli, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add new generic sorting icons and rename existing alphabetic sorting icons to -name scheme

[Emirald Mateli]

emateli added a comment.


  @trickyricky26 I think we should stick to whatever conventions the KDE 
community has. I checked with Dolphin and the arrow goes up for descending and 
up for ascending, however the VDG makes these calls in the end.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D16905

To: trickyricky26, #vdg, #breeze, ndavis
Cc: emateli, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D16905: Add new generic sorting icons and rename existing alphabetic sorting icons to -name scheme

[Emirald Mateli]

emateli added a comment.


  1. Aren't the names in reverse? `view-sort-ascending` shows a list in 
descending order
  2. In `view-sort-descending` the items are not even sorted. Not sure how i 
feel about that.

REPOSITORY
  R266 Breeze Icons

BRANCH
  add-sort-options-icon (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D16905

To: trickyricky26, #vdg, #breeze, ndavis
Cc: emateli, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 45438.
emateli marked 9 inline comments as done.
emateli added a comment.


  - create on stack
  - upd define
  - remove unused import
  - use qstringliteral
  - remove unused export
  - match file name
  - remove import prefixes
  - remove q_unused on used parameter
  - use fwd class declaration
  - reserve space for items
  - Do not use _p for cpp files
  - simplify expressions
  - add cpp files w/o _p
  - optimize imports

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44938=45438

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44938.
emateli added a comment.


  - Initialize pointers to nullptr
  - Code style fix
  
  ---
  
  Thoughts on proceeding with the proposed changes to batchrenamejob? Also, the 
filenameutils namespace feels like duplicated work. Any existing solution 
available to use? It uses QMimeDatabase to look up the extension and if its not 
found, then it uses the suffix from QFileInfo

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44737=44938

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  Also, thoughts on my previous comments regarding `BatchRenameJob`?

INLINE COMMENTS

> mlaurent wrote in batchrenamedialog.h:69
> it's a good idea to initialize to nullptr each pointer here.
> So we can see problem easily.

You mean to initialize them to nullptr on the header file or using a 
constructor initialization list? Also see what problem?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44737.
emateli marked 4 inline comments as done.
emateli added a comment.


  - coding style fix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44218=44737

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli marked 2 inline comments as done.
emateli added a comment.


  Something that might need a bit of input is `BatchRenameJob`.
  
  - This was added a while back so that Dolphin doesn't do a batch rename as a 
series of single file renames.
  - This job takes care of replacing the `#` placeholder as well.
  - What this patch really wants is an API to rename a list of files, into some 
new names without doing any additional handling except moving the files (as the 
placeholder replacement is done elsewhere).
  - By consulting 
https://lxr.kde.org/search?_filestring=&_string=BatchRenameJob we can see that 
Dolphin is the only application to make use of this `BatchRenameJob` API.
  - Leaving credit to the original author, since the `#` placeholder will not 
be used anymore. Should we re-purpose this class to accept two lists, one with 
the current names, the other with the new ones and remove the original 
placeholder implementation (as it would be mostly dead code at this point) 
while keeping everything else intact.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44218.
emateli marked 3 inline comments as done.
emateli added a comment.


  - Remove QtCore/

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44217=44218

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44217.
emateli added a comment.


  - fix indent
  - remove setlayout call
  - Add missing parent parameters
  - Remove unnecessary QtCore/ prefix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44189=44217

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44189.
emateli added a comment.


  - use _p notation. Add BatchRenameDialog to installable headers
  
  ---
  
  Besides any current review, the last step for this patch would be to actually 
use the BatchRenameJob already in KIO. Regarding that: Should I put it into a 
separate patch and make this depend on it or continue working here?

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=42324=44189

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  In D14631#303886 , @aacid wrote:
  
  > As far as i can see none of these headers get installed so should they all 
be renamed to _p.h ?
  >
  > Also if the headers don't get installed how do you use the new classes?
  
  
  @aacid Would be great if you'd point me towards making them installable. Only 
the dialog itself should be exported.
  @dfaure Do functions for getting file suffix and name already exist in KIO or 
other KDE libraries? The whole filenameutils feels like duplicate work.
  
  I want to give KDevelop a spin for this, how do I import a new formatter so 
it can format my code according to the KDE guidelines.

INLINE COMMENTS

> dfaure wrote in batchrenametypes.h:32
> This looks more like a namespace than an actual class, given that everything 
> is static.
> 
> Alternatively (and even better), make capturedGroups and the two associated 
> methods non-static, meaning that one has to instanciate the class in order to 
> use it. This is only used in the dialog, right? So there's no need for this 
> "global" variable, it can just be a member of BatchRenameTypes which can be a 
> member of the dialog, AFAICS.

I tried the instance route but then I wouldn't be able to pass the method as a 
callback. Current way is not ideal either. Will figure something out.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 42324.
emateli marked 15 inline comments as done.
emateli added a comment.


  - Code style fixes
  - use .insert instead of []
  - Indent Q_OBJECT
  - use isEmpty instead of len==0
  - use aggregate initialization
  - protected -> private
  - added parent check
  - use functions instead of QString fields
  - delete itemData
  - code format
  - FileNameUtils converted to namespace
  - remove unnecessary using

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=39137=42324

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli retitled this revision from "Migrate D10698 from Dolphin to KIO" to 
"Adds a new RenameDialog to KIO with more options for batch renaming".
emateli edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14631: Migrate D10698 from Dolphin to KIO

[Emirald Mateli]

emateli added a reviewer: Frameworks.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14631: Migrate D10698 from Dolphin to KIO

[Emirald Mateli]

emateli created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
emateli requested review of this revision.

REPOSITORY
  R241 KIO

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10937: Retouching of Screen Layout Selection OSD Icons

[Emirald Mateli]

emateli added a comment.


  Hi, while I do appreciate the icons I also have a question: Why the use of 
laptop and projector over of two simple generic screens. Wouldn't that be 
better for all use cases?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D10937

To: pstefan, #breeze
Cc: emateli, abetts, mart, ngraham, #frameworks, davidc, michaelh, crozbo, 
firef, alexeymin, skadinna, aaronhoneycutt, mbohlender

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli abandoned this revision.
emateli added a comment.


  Resolved in https://phabricator.kde.org/D9217

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8920

To: emateli, #frameworks, dfaure
Cc: #frameworks

D9217: KUriFilter: don't return an error on non-existing files.

[Emirald Mateli]

emateli added a comment.


  I can't seem to be able to properly run this(whatever changes I make to the 
kshorturifilter.cpp seem to be ignored?!) but on paper looks good to me. Do we 
mark https://phabricator.kde.org/D8920 as abandoned, or wait for this to commit 
then the new `dir` property of KUrlNavigator can be pushed as well.

REVISION DETAIL
  https://phabricator.kde.org/D9217

To: dfaure, emateli, elvisangelaccio
Cc: broulik, #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli updated this revision to Diff 23532.
emateli added a comment.


  - ~/ at the start always resolves to $HOME

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8920?vs=22702=23532

BRANCH
  relative-files-v2

REVISION DETAIL
  https://phabricator.kde.org/D8920

AFFECTED FILES
  autotests/kurlnavigatortest.cpp
  src/filewidgets/kurlnavigator.cpp
  src/filewidgets/kurlnavigator.h
  src/widgets/kurifilter.cpp
  src/widgets/kurifilter.h

To: emateli, #frameworks, dfaure
Cc: #frameworks

D9100: KUriFilter: simplify data structures, fix memory leak

[Emirald Mateli]

emateli added inline comments.

INLINE COMMENTS

> kurifilter.cpp:597
> +if (filters.isEmpty() || filters.contains(plugin->objectName())) {
> +if (plugin->filterUri(data)) {
>  filtered = true;

Perhaps this can be merged with the if statement above.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9100

To: dfaure, apol, davidedmundson, arichardson, bshah
Cc: emateli, #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli added a comment.


  Ping. Thoughts on the last comment?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8920

To: emateli, #frameworks, dfaure
Cc: #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli added a comment.


  @dfaure I've managed to take another look at this, added a `directory` 
property which controls the base dir. Works as it should, however there is one 
case where I'm not sure which way we want it and is probably just a matter of 
preference.
  
  User types `~/some-dir` which does not exist. The `kshorturifilter` plugin 
will expand that to `/home/.../some-dir`, check that it doesn't exist, and then 
decide to not return the expanded value, but rather the original value.
  The result of this is that the `KUrlNavigator` will attempt to browse 
`/home/.../~/some-dir` instead of `/home/.../some-dir` which perhaps might be 
slightly confusing, this works as it should if the directory does exist though.
  On the upside directories named `~` are browsable. Thoughts? Do we try to 
"fix" this or is the current behaviour okay.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8920

To: emateli, #frameworks, dfaure
Cc: #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli updated this revision to Diff 22702.
emateli added a comment.


  - remove QDir::homePath + minor code style

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8920?vs=22671=22702

BRANCH
  relative-files

REVISION DETAIL
  https://phabricator.kde.org/D8920

AFFECTED FILES
  autotests/kurlnavigatortest.cpp
  src/filewidgets/kurlnavigator.cpp
  src/widgets/kurifilter.cpp

To: emateli, #frameworks, dfaure
Cc: #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli added a reviewer: Frameworks.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8920

To: emateli, #frameworks
Cc: #frameworks

D8920: Fixes url navigation with relative links on KUrlNavigator

[Emirald Mateli]

emateli created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This patch aims to address an issue with KUrlNavigator using relative paths. 
Upon attempting to navigate using relative paths, due to the way 
`QUrl::fromUserInput` works, a relative symbol such as 'Documents' will be 
converted to `http://Documents` or `.config` will be converted to an empty 
QUrl. This patch uses the `AssumeLocalFile` flag along with the home directory 
as the relative link (which is where the suggestions are taken from), alongside 
that it adds two test cases to account for this behaviour. The suggestions 
would perhaps be more intuitive if they were from the current directory but I 
assume that's an issue to be discussed in another patch.
  
  All tests currently pass, but someone with more knowledge of this should 
check if this patch potentially breaks something with `KUriFilter::filterUri`.

TEST PLAN
  Open an application where KUrlNavigator is used. Eg.: Dolphin, Gwenview
  
  - Select the KUrlNavigator and clear the text
  - Type something with dot or a character that would otherwise show suggestions
  - Highlight one of the suggestions and activate it via enter
  
  Expected: The selected directory should be browsed
  Actual: Dolphin will show invalid protocol error for dotfiles or otherwise 
launch browser. Gwenview fails silently.

REPOSITORY
  R241 KIO

BRANCH
  relative-files

REVISION DETAIL
  https://phabricator.kde.org/D8920

AFFECTED FILES
  autotests/kurlnavigatortest.cpp
  src/filewidgets/kurlnavigator.cpp
  src/widgets/kurifilter.cpp

To: emateli
Cc: #frameworks

D7828: fix createKMessageBox focus widget inconsistency

[Emirald Mateli]

emateli added a comment.


  Ping @subdiff @abetts does this iteration work for you guys? It's marked as 
ready to land but I feel that we should get an overall opinion on this.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg, rkflx
Cc: elvisangelaccio, rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: fix createKMessageBox focus widget inconsistency

[Emirald Mateli]

emateli retitled this revision from "fix createKMessageBox focus button 
inconsistency" to "fix createKMessageBox focus widget inconsistency".

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg, rkflx
Cc: elvisangelaccio, rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: fix createKMessageBox focus button inconsistency

[Emirald Mateli]

emateli retitled this revision from "createKMessageBox tries to focus a default 
button when available" to "fix createKMessageBox focus button inconsistency".
emateli edited the summary of this revision.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg, rkflx
Cc: elvisangelaccio, rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli updated this revision to Diff 21453.
emateli added a comment.


  Well, the diff of this patch took a sizeable reduction.
  
  I took a second look at this and here is what happened in the end and what 
caused the inconsistency with Dolphin.
  
  The issue was that the `QDialogButtonBox` argument, which contains the 
buttons needs to have a not null parent argument. If it does, then everything 
works as expected, if it doesn't (which Dolphin does not suppliy) then the 
focus is all messed up and will be given in order to the widgets created that 
can be focused.
  
  From what I dug out the order seems to be:
  
  1. Label (if the `AllowLink` flag is present)
  2. The string list
  3. Checkbox
  4. Left most action button
  
  Moving the call of `setParent` before any of the above mentioned widgets is 
created seems to fix this (I built this system-wide so all the KDE apps running 
can use it and no issues so far, although it's only been a couple of hours)
  
  If this turns out to be okay, I'll edit the summary and title later, since it 
is re-purposed in a way by not involving widget focus change.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7828?vs=20497=21453

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  src/kmessagebox.cpp

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  I think this is getting a bit out of hand here. Please try to read my last 
message where I explain why I decided to submit this patch and why I think it's 
a bug (the whole parent widget thing).
  
  Given the new information on why this occurs the whole focus by force thing 
might not be optimal (although I don't see it as being wrong either), so we can 
check for a solution that perhaps simply fixes the inconsistency when being 
called with a parent and without one.
  This could be easily solvable in Dolphin's repository where this would need 
only one additional parameter to the `createKMessageBox` call, but I still 
thing it belongs to be fixed in the frameworks.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  Hey @subdiff, thanks for your input on this. Whether this patch goes in or 
not, I still think that this "odd" behaviour is something that the frameworks 
should fix or change rather than relying on the developer of each application 
to do this.
  
  However I never expected this to stir up such a conversation given that I 
thought this was the normal behaviour in Plasma, given that all(?) the other 
dialogs created by the framework have the default button focused (See: Konsole 
close dialog w/ multiple tabs open (the exact same dialog as Dolphin's), or 
Konversation after having joined a channel. )
  
  Since these functions internally all rely on `createKMessageBox` and work 
properly so to say, I dug a bit deeper into this and found out the following 
why Dolphin's close dialog is the odd one out: If no parent window is specified 
when calling this function then the checkbox will be focused, if one is passed 
then the focus shifts onto the buttons.
  
  Current call: `QDialogButtonBox* buttons = new 
QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | 
QDialogButtonBox::Cancel);`
  "Fixed" call: `QDialogButtonBox* buttons = new 
QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | 
QDialogButtonBox::Cancel, dialog);`
  
  This leads me to believe that the checkbox having the focus is an 
irregularity, whether one would consider it a bug is a matter of prespective. 
But i would expect the frameworks to be consistent in what it does and whether 
the dialog has a parent or not should not affect what widget is focused.
  
  But just like @aacid mentioned, we should not look to break existing code, so 
given the new information on why this occurs, then perhaps a new implementation 
might be needed or simply a fix for this change in behaviour when a parent 
widget is present or not.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  @aacid the issue at hand is that in this particular scenario the default 
button more or less equals to the one that has focus.
  
  There are only 3 types of widgets that can be created here: buttons, a list 
or a check box.
  
  You can't interact with the list so it is ruled out for being focused. The 
check box acts as a confirmation and thus is a secondary element which you also 
do not want to toggle by mistake, so it rests upon the buttons to have it.
  If any button is to have focus then it might as well be the default one,  
giving it to another button will break the enter button since the focused 
button's handler will be invoked over the default's. 
  This also has the added benefit of allowing to invoke the default action via 
space bar.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  In my opinion a "Default" widget, should also have focus. Makes little sense 
to have some widget focused and then pressing enter to trigger the action of 
another one. 
  By focusing, the default action is also made visible and allows to invoke the 
it action by pressing space bar. And no, I haven't spoken to the usability 
team. What is the team name so we can tag them here perhaps?
  
  After running some tests even when manually setting some other action to 
default, enter always invokes the same action. Eg: in the case of a Yes, No, 
Cancel dialog, the yes will always be called regardless.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli updated this revision to Diff 20497.
emateli added a comment.


  Added autotests

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7828?vs=19543=20497

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kmessageboxautotest.cpp
  autotests/kmessageboxautotest.h
  src/kmessagebox.cpp

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  @aacid just to make clear, you're saying to add another case to the 
`testMessageBox` on `kmessageboxtest.cpp` right?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli edited the summary of this revision.
emateli edited the test plan for this revision.
emateli added a reviewer: Frameworks.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  createKMessageBox-btn-focus

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  src/kmessagebox.cpp

To: emateli
Cc: #frameworks