D13921: Don't show confirmation dialog for Trash action by default

[Алексей Шилин]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:7d15dad5e427: Dont show confirmation dialog for 
Trash action by default (authored by aleksejshilin).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13921?vs=37242=37441

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

AFFECTED FILES
  src/widgets/jobuidelegate.cpp

To: aleksejshilin, dfaure, #frameworks, #dolphin, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D13921: Don't show confirmation dialog for Trash action by default

[Алексей Шилин]

aleksejshilin edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  dont_confirm_trash

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

To: aleksejshilin, dfaure, #frameworks, #dolphin, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D13921: Don't show confirmation dialog for Trash action by default

[Алексей Шилин]

aleksejshilin added a comment.


  In D13921#288012 , @ngraham wrote:
  
  > There's a problem with the git setup on the computer you used to make this 
commit; the `name` field appears to be empty, so I get an error when I try to 
apply the patch
  
  
  That's weird. It is the very same machine which I previously used to submit 
all of my revisions, and actual commits look fine, too 
.
 I'll look into it tomorrow when I get to that PC.
  
  In D13921#288014 , @ngraham wrote:
  
  > Are you planning to update `konqueror/settings/konq/behaviour.cpp` as the 
comment asked? ;)
  
  
  As @dfaure said, these things are no longer there, that's why I had changed 
the comment to point to the new location.
  
  In D13921#288178 , @ngraham wrote:
  
  > Do you have commit access yet?
  
  
  I do, thanks! Will land tomorrow.

REPOSITORY
  R241 KIO

BRANCH
  dont_confirm_trash

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

To: aleksejshilin, dfaure, #frameworks, #dolphin, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D13921: Don't show confirmation dialog for Trash action by default

[Алексей Шилин]

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

REVISION SUMMARY
  Dolphin and KIO default confirmation settings were not in sync: the former
  defaults to not showing the dialog while the latter used to default to the
  opposite.
  
  This change resolves the inconsistency by adjusting KIO defaults.
  
  BUG: 385492

TEST PLAN
  1. Create a new user account and log into it.
  2. Open Dolphin.
  3. Create a text file and hit Del. The confirmation dialog should *not* 
appear.
  4. Check that permanent deletion (Shift+Del) and emptying the Trash still 
show the confirmation dialog.

REPOSITORY
  R241 KIO

BRANCH
  dont_confirm_trash

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

AFFECTED FILES
  src/widgets/jobuidelegate.cpp

To: aleksejshilin, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:47340d6a554c: concatPaths: process empty path1 correctly 
(authored by aleksejshilin).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12490?vs=32961=33260

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

AFFECTED FILES
  src/pathhelpers_p.h

To: aleksejshilin, #frameworks, anthonyfieroni, dfaure
Cc: michaelh, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

aleksejshilin added a comment.


  In D12490#252959 , @anthonyfieroni 
wrote:
  
  > path1 *should* never be empty
  
  
  Hm... I thought concatPaths() is supposed to be used like this: 
`concatPaths(basePath, relativePath)` - then an empty basePath makes sense (and 
means 'current directory'). If this is not the case, then:
  
  - KUrlCompletion (and probably KCompletion) needs to be fixed;
  - `Q_ASSERT(!path1.isEmpty())` should be added here so that such bugs get 
noticed.
  
  Is this correct?

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, anthonyfieroni, dfaure
Cc: michaelh, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

aleksejshilin added reviewers: anthonyfieroni, dfaure.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, anthonyfieroni, dfaure
Cc: michaelh, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

aleksejshilin requested review of this revision.
aleksejshilin added a comment.


  Here is an example of KUrlCompletion breakage.
  Before, it returned invalid absolute paths like /Desktop etc.:
  F5820341: Before.png 
  After:
  F5820340: After.png 

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: michaelh, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

aleksejshilin created this revision.
aleksejshilin added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
aleksejshilin requested review of this revision.

REVISION SUMMARY
  When path1 was empty (i.e. 'current directory'), concatPaths() used to
  return an absolute path /path2 instead of just path2, which broke e.g.
  KUrlCompletion in certain cases.
  
  This commit makes concatPaths() return just path2 if path1 is empty.

REPOSITORY
  R241 KIO

BRANCH
  fix_concatPaths

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

AFFECTED FILES
  src/pathhelpers_p.h

To: aleksejshilin, #frameworks
Cc: michaelh, bruns

D12490: concatPaths: process empty path1 correctly

[Алексей Шилин]

aleksejshilin planned changes to this revision.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: michaelh, bruns

D11331: add gaming_input devices and others to Battery

[Алексей Шилин]

aleksejshilin added a comment.


  In D11331#226428 , @dollinger 
wrote:
  
  > Okay :) But what's about the other entries, like `UP_DEVICE_KIND_TABLET`,  
`UP_DEVICE_KIND_MEDIA_PLAYER`?
  >  Is there a reason why they are excluded?
  
  
  The reason is probably that they are missing in the documentation 
 (which is likely 
a bug).

REPOSITORY
  R245 Solid

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

To: dollinger, broulik, #plasma
Cc: aleksejshilin, #frameworks, michaelh, ngraham

D11331: add gaming_input devices and others to Battery

[Алексей Шилин]

aleksejshilin added inline comments.

INLINE COMMENTS

> upowerdevice.cpp:98
> + */
> +return (uptype != 0 && uptype != 1 && uptype != 4);
>  default:

So you're changing from 'whitelist' to 'blacklist' behavior here. What will 
happen when another UPower device type is added by the upstream?

REPOSITORY
  R245 Solid

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

To: dollinger, broulik, #plasma
Cc: aleksejshilin, #frameworks, michaelh, ngraham

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:37868c2c57f7: Fix protocol selection in KUrlNavigator 
(authored by aleksejshilin).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10993?vs=28571=28574

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

AFFECTED FILES
  src/filewidgets/kurlnavigator.cpp

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

aleksejshilin added inline comments.

INLINE COMMENTS

> dfaure wrote in kurlnavigator.cpp:349
> Well, OK, it's not that it's *incorrect* for file:, it just seems unnecessary 
> to make that method call. So given that we have an if() already, why not only 
> do it in the else?

> Well, OK, it's not that it's *incorrect* for file:, it just seems unnecessary 
> to make that method call.

Ah, I misunderstood what you meant.

> So given that we have an if() already, why not only do it in the else?

Done.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

aleksejshilin updated this revision to Diff 28571.
aleksejshilin added a comment.


  - Move setting authority to the else branch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10993?vs=28519=28571

BRANCH
  fix_kurlnavigator_protocol_selection

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

AFFECTED FILES
  src/filewidgets/kurlnavigator.cpp

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

aleksejshilin updated this revision to Diff 28519.
aleksejshilin added a comment.


  - Add comment explaining the empty authority

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10993?vs=28496=28519

BRANCH
  fix_kurlnavigator_protocol_selection

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

AFFECTED FILES
  src/filewidgets/kurlnavigator.cpp

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

aleksejshilin added inline comments.

INLINE COMMENTS

> dfaure wrote in kurlnavigator.cpp:349
> Is this in order to get smb:// instead of smb: ? It looks correct, but not 
> for "file", how about moving that to an else{} branch of the if below? And I 
> could add a comment like "we want smb:// or ftp://, not just smb: or ftp:".

> Is this in order to get smb:// instead of smb: ?

Yes.

> It looks correct, but not for "file",

Why not? If I read RFC 8089 correctly, for host rule it references RFC 3986 
which allows it to be empty, i.e. an empty authority is valid.

> And I could add a comment like "we want smb:// or ftp://, not just smb: or 
> ftp:".

Makes sense, indeed. Will do shortly.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D10993: Fix protocol selection in KUrlNavigator

[Алексей Шилин]

aleksejshilin created this revision.
aleksejshilin added reviewers: Frameworks, dfaure.
Restricted Application added a project: Frameworks.
aleksejshilin requested review of this revision.

REVISION SUMMARY
  QUrl no longer accepts an empty authority and a non-empty path that
  starts with '//' (Qt commit f62768d046528636789f901ac79e2cfa1843a7b7).
  As the result, protocol selection using the menu no longer worked for
  all but 'file' scheme.

REPOSITORY
  R241 KIO

BRANCH
  fix_kurlnavigator_protocol_selection

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

AFFECTED FILES
  src/filewidgets/kurlnavigator.cpp

To: aleksejshilin, #frameworks, dfaure
Cc: michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a comment.


  In D8958#212246 , @ngraham wrote:
  
  > @aleksejshilin doesn't have commit access
  
  
  Actually, I do now. :)

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:4a6a3b81bedc: Fix unintentional breadcrumb menu item 
activation (authored by aleksejshilin).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8958?vs=26872=27866

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

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

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

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a comment.


  In D8958#209050 , @dfaure wrote:
  
  > Instead of the bool member, isn't it enough to test the distance again in 
mouseReleaseEvent? AFAIK that's how most widget do it. It also leads to one 
difference of behaviour in case someone moves the mouse a bit and then back to 
the original position, in that case the mouseReleaseEvent considers that no 
move happened (not sure if that's what you want here).
  
  
  If user moved the mouse considerably, then it's assumed to be intentional, 
and the subsequent release event shouldn't be ignored even if the final 
position is the same as the initial one. This way it is consistent with 
drag'n'drop behavior.
  
  Besides, only the first mouse release event should be ignored, so a flag is 
needed anyway.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks, dfaure
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a comment.


  In https://phabricator.kde.org/D8958#203931, @ngraham wrote:
  
  > OK cool. The patch works for me! +1.
  
  
  Great, thanks a lot!
  
  > Get a code review from someone else before committing.
  
  I don't have push access, so no worries. :)

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added inline comments.

INLINE COMMENTS

> ngraham wrote in kurlnavigatormenu.cpp:85
> Will this still work if the user is using the mouse in left-handed mode?

Quoting Qt documentation [1]:

> | Qt::LeftButton | The left button is pressed, or an event refers to the left 
> button. (**The left button may be the right button on left-handed mice.**) |
> |

Should work just fine.

[1] http://doc.qt.io/qt-5/qt.html#MouseButton-enum

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin updated this revision to Diff 26872.
aleksejshilin added a comment.


  - Don't consider mouse moves which are smaller than drag distance
  - Don't pass the ignored mouse release event to the base class

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8958?vs=22866=26872

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

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

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a comment.


  Hi again. :)
  Any news on this? Is there anything I should improve?

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: broulik, ngraham, michaelh

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin added a comment.


  So, any comments on the updated revision?

REPOSITORY
  R241 KIO

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

To: aleksejshilin, #frameworks
Cc: broulik, ngraham

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin updated this revision to Diff 22866.
aleksejshilin added a comment.


  - Don't consider mouse moves which are smaller than drag distance
  - Don't pass the ignored mouse release event to the base class

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8958?vs=22788=22866

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

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

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks
Cc: broulik, ngraham

D8958: Fix unintentional breadcrumb menu item activation

[Алексей Шилин]

aleksejshilin created this revision.
aleksejshilin added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Since breadcrumb menu is opened on mouse press, it may receive
  the corresponding mouse release event which may unintentionally
  activate the random item which happens to be under mouse pointer.
  
  This commit makes the menu ignore the first mouse release event
  unless the pointer was moved. It fixes the issue and still allows
  'Press mouse button' -> 'Move to an item' -> 'Release the button'
  usage scenario.
  
  BUG: 380287

REPOSITORY
  R241 KIO

BRANCH
  fix_accidental_breadcrumb_menu_item_activation

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

AFFECTED FILES
  src/filewidgets/kurlnavigatormenu.cpp
  src/filewidgets/kurlnavigatormenu_p.h

To: aleksejshilin, #frameworks