Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review886 --- Ship it! Aside from a couple of minor nitpicks below I think the patch looks good now, so please commit it after taking care of those :) /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment555 Don't forget to remove this before committing ;) /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment554 Too much indentation here. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment552 Please add a comment here saying that the fall-through is intentional. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment556 This closing brace should be at the same level as the else statement on the next line. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment553 Too much indentation in this block. - Fredrik On 2009-04-08 21:33:17, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-08 21:33:17) Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/abstractitemview.h 951365 /trunk/KDE/kdebase/apps/plasma/applets/folderview/abstractitemview.cpp 951365 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 951365 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 951365 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-04-02 13:56:47, Fredrik Höglund wrote: I think in general the code looks good, but there are still numerous coding style issues, especially with the way the code is indented. Shantanu Tushar Jha wrote: Oh, I apologise for that, but I'm unable to figure out where I've messed up with the indentation. Please point few examples where there's a problem, it'll be lots of help. Fredrik Höglund wrote: You indent the lines with 2 spaces, while the coding style specifies 4. The coding style is documented in detail here: http://techbase.kde.org/Policies/Kdelibs_Coding_Style Oops, I didn't pay attention to that in the code, sorry. Will change it now. On 2009-04-02 13:56:47, Fredrik Höglund wrote: /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1344 http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1344 This could result in a division by zero. wrote: The only place where m_columns is set to 0 is the constructor, but after the layout is done, there should be at least one column and one row, then how m_column can be zero? m_columns is used to hold the number of columns visible, isn't it? wrote: The layout isn't done until items are actually inserted into the model, so until that happens m_columns will be 0. So should I just return; the keyPressEvent function in the beginning if m_columns is zero - if (m_columns == 0) return; ? On 2009-04-02 13:56:47, Fredrik Höglund wrote: /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1372 http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1372 This code still doesn't take the flow into account. The icons can flow from left to right, right to left, top to bottom and so on, as indicated by m_flow. wrote: I can't figure out much what does flow mean here, only that it might mean that the model might be displayed in reverse order for some value of m_flow. What exactly is it doing, is unclear to me as I couldn't find some way to set the flow from the applet config and see the effect. Please hint on this. Thanks a lot for the help :) wrote: The flow can only be configured in the config dialog when the applet is used as a containment, but basically it controls how the icons are laid out in the view, like this: LeftToRight: [1][2][3] [4][5][6] [7][8][9] RightToLeft: [3][2][1] [6][5][4] [9][8][7] TopToBottom: [1][4][7] [2][5][8] [3][6][9] Thanks, actually I figured this out today morning by manually using setFlow :) I'll make the code according to it. - Shantanu --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review796 --- On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-02 13:21:02) Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-04-02 13:56:47, Fredrik Höglund wrote: I think in general the code looks good, but there are still numerous coding style issues, especially with the way the code is indented. Shantanu Tushar Jha wrote: Oh, I apologise for that, but I'm unable to figure out where I've messed up with the indentation. Please point few examples where there's a problem, it'll be lots of help. You indent the lines with 2 spaces, while the coding style specifies 4. The coding style is documented in detail here: http://techbase.kde.org/Policies/Kdelibs_Coding_Style On 2009-04-02 13:56:47, Fredrik Höglund wrote: /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1344 http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1344 This could result in a division by zero. wrote: The only place where m_columns is set to 0 is the constructor, but after the layout is done, there should be at least one column and one row, then how m_column can be zero? m_columns is used to hold the number of columns visible, isn't it? The layout isn't done until items are actually inserted into the model, so until that happens m_columns will be 0. On 2009-04-02 13:56:47, Fredrik Höglund wrote: /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1372 http://reviewboard.kde.org/r/368/diff/4/?file=4671#file4671line1372 This code still doesn't take the flow into account. The icons can flow from left to right, right to left, top to bottom and so on, as indicated by m_flow. wrote: I can't figure out much what does flow mean here, only that it might mean that the model might be displayed in reverse order for some value of m_flow. What exactly is it doing, is unclear to me as I couldn't find some way to set the flow from the applet config and see the effect. Please hint on this. Thanks a lot for the help :) The flow can only be configured in the config dialog when the applet is used as a containment, but basically it controls how the icons are laid out in the view, like this: LeftToRight: [1][2][3] [4][5][6] [7][8][9] RightToLeft: [3][2][1] [6][5][4] [9][8][7] TopToBottom: [1][4][7] [2][5][8] [3][6][9] - Fredrik --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review796 --- On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-02 13:21:02) Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-02 08:55:49.469863) Review request for Plasma. Changes --- Provided support for keyboard navigation even if the user has made the view unsorted. Please try it out and report any issues that you may encounter. I've tried to adhere with the kdelibs coding style, hope its all right. :) Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 947761 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 947761 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
A problem with the way this function is implemented is that it assumes
that the view is always sorted and that the icons always flow from left to
right.
When the user has rearranged the icons (m_layoutBroken is true), you
have to assume that the icons are no longer arranged in a grid and that the
visual order no longer matches the order in the model.
When this is the case, and the user has pressed the up key for example,
you have to iterate over all the icons and find the one that is closest to
the current icon while still being above it.
wrote:
you have to iterate over all the icons. I'm working on this by
iterating all icons and finding the nearest one to the current selection
according to the key pressed, but the code is getting really complex in terms
of calculations. I was wondering if there is any other way of doing this? If
anyone has an idea, please let me know. Till then I'm working on it.
wrote:
I would do something like this (the example is for the up key only):
QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;
for (int i = 0; i m_validRows; i++) {
const QModelIndex index = m_model-index(i, 0);
const QPoint pos = visualRect(index).center();
if (pos.y() currentPos.y()) {
int distance = (pos - currentPos).manhattanLength();
if (distance lastDistance || !currentIndex.isValid()) {
nextIndex = index;
lastDistance = distance;
}
}
}
If nextIndex is valid when you get here, it's the index you should move
to.
If it isn't valid there are no icons above the current icon.
Thanks for working on this feature :)
wrote:
Ok, thanks for the help, that was less complex then mine ;) Its almost
done, just a few minor issues remaining.
But, I'm unable to understand why you're using `!currentIndex.isValid()`
in if (distance lastDistance || !currentIndex.isValid()) ?
Why do we need to validate the currentIndex ?
Oh right, that should actually be nextIndex, not currentIndex.
I guess should read what I wrote more carefully before pressing publish :)
- Fredrik
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-04-02 08:55:49, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-04-02 08:55:49)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 947761
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 947761
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
A problem with the way this function is implemented is that it assumes
that the view is always sorted and that the icons always flow from left to
right.
When the user has rearranged the icons (m_layoutBroken is true), you
have to assume that the icons are no longer arranged in a grid and that the
visual order no longer matches the order in the model.
When this is the case, and the user has pressed the up key for example,
you have to iterate over all the icons and find the one that is closest to
the current icon while still being above it.
wrote:
you have to iterate over all the icons. I'm working on this by
iterating all icons and finding the nearest one to the current selection
according to the key pressed, but the code is getting really complex in terms
of calculations. I was wondering if there is any other way of doing this? If
anyone has an idea, please let me know. Till then I'm working on it.
wrote:
I would do something like this (the example is for the up key only):
QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;
for (int i = 0; i m_validRows; i++) {
const QModelIndex index = m_model-index(i, 0);
const QPoint pos = visualRect(index).center();
if (pos.y() currentPos.y()) {
int distance = (pos - currentPos).manhattanLength();
if (distance lastDistance || !currentIndex.isValid()) {
nextIndex = index;
lastDistance = distance;
}
}
}
If nextIndex is valid when you get here, it's the index you should move
to.
If it isn't valid there are no icons above the current icon.
Thanks for working on this feature :)
wrote:
Ok, thanks for the help, that was less complex then mine ;) Its almost
done, just a few minor issues remaining.
But, I'm unable to understand why you're using `!currentIndex.isValid()`
in if (distance lastDistance || !currentIndex.isValid()) ?
Why do we need to validate the currentIndex ?
wrote:
Oh right, that should actually be nextIndex, not currentIndex.
I guess should read what I wrote more carefully before pressing publish
:)
But still, why we need to validate (or, invalidate) nextIndex
`!currentIndex.isValid()` ? I tried to figure out, but failed. A little help
here :)
- Shantanu
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-04-02 08:55:49, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-04-02 08:55:49)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 947761
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 947761
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-02 13:21:02.238361) Review request for Plasma. Changes --- Its ok, I figured it out why was !nextIndex.isValid() required and have updated the Diff accordingly. Please check it and see if there are any issues and if this is ok to commit ... Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review796 --- I think in general the code looks good, but there are still numerous coding style issues, especially with the way the code is indented. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h http://reviewboard.kde.org/r/368/#comment486 I'd prefer it if this function was in AbstractItemView instead, since the code will work for the ListView class as well. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment489 IconView already sets the focus policy to StrongFocus at the top of the constructor (as of today). /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment487 This could result in a division by zero. /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp http://reviewboard.kde.org/r/368/#comment488 This code still doesn't take the flow into account. The icons can flow from left to right, right to left, top to bottom and so on, as indicated by m_flow. - Fredrik On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-04-02 13:21:02) Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
A problem with the way this function is implemented is that it assumes
that the view is always sorted and that the icons always flow from left to
right.
When the user has rearranged the icons (m_layoutBroken is true), you
have to assume that the icons are no longer arranged in a grid and that the
visual order no longer matches the order in the model.
When this is the case, and the user has pressed the up key for example,
you have to iterate over all the icons and find the one that is closest to
the current icon while still being above it.
wrote:
you have to iterate over all the icons. I'm working on this by
iterating all icons and finding the nearest one to the current selection
according to the key pressed, but the code is getting really complex in terms
of calculations. I was wondering if there is any other way of doing this? If
anyone has an idea, please let me know. Till then I'm working on it.
wrote:
I would do something like this (the example is for the up key only):
QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;
for (int i = 0; i m_validRows; i++) {
const QModelIndex index = m_model-index(i, 0);
const QPoint pos = visualRect(index).center();
if (pos.y() currentPos.y()) {
int distance = (pos - currentPos).manhattanLength();
if (distance lastDistance || !currentIndex.isValid()) {
nextIndex = index;
lastDistance = distance;
}
}
}
If nextIndex is valid when you get here, it's the index you should move
to.
If it isn't valid there are no icons above the current icon.
Thanks for working on this feature :)
wrote:
Ok, thanks for the help, that was less complex then mine ;) Its almost
done, just a few minor issues remaining.
But, I'm unable to understand why you're using `!currentIndex.isValid()`
in if (distance lastDistance || !currentIndex.isValid()) ?
Why do we need to validate the currentIndex ?
wrote:
Oh right, that should actually be nextIndex, not currentIndex.
I guess should read what I wrote more carefully before pressing publish
:)
wrote:
But still, why we need to validate (or, invalidate) nextIndex
`!currentIndex.isValid()` ? I tried to figure out, but failed. A little help
here :)
Because in the first iteration of the loop, lastDistance is 0, so 'distance
lastDistance' will be false.
Adding '|| nextIndex.isValid()' causes nextIndex and lastDistance to be
initialized to the values for the first item in the view.
Assigning QModelIndex() to an index makes the index invalid.
- Fredrik
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-04-02 13:21:02, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-04-02 13:21:02)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 948236
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 948236
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
A problem with the way this function is implemented is that it assumes
that the view is always sorted and that the icons always flow from left to
right.
When the user has rearranged the icons (m_layoutBroken is true), you
have to assume that the icons are no longer arranged in a grid and that the
visual order no longer matches the order in the model.
When this is the case, and the user has pressed the up key for example,
you have to iterate over all the icons and find the one that is closest to
the current icon while still being above it.
wrote:
you have to iterate over all the icons. I'm working on this by
iterating all icons and finding the nearest one to the current selection
according to the key pressed, but the code is getting really complex in terms
of calculations. I was wondering if there is any other way of doing this? If
anyone has an idea, please let me know. Till then I'm working on it.
wrote:
I would do something like this (the example is for the up key only):
QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;
for (int i = 0; i m_validRows; i++) {
const QModelIndex index = m_model-index(i, 0);
const QPoint pos = visualRect(index).center();
if (pos.y() currentPos.y()) {
int distance = (pos - currentPos).manhattanLength();
if (distance lastDistance || !currentIndex.isValid()) {
nextIndex = index;
lastDistance = distance;
}
}
}
If nextIndex is valid when you get here, it's the index you should move
to.
If it isn't valid there are no icons above the current icon.
Thanks for working on this feature :)
Ok, thanks for the help, that was less complex then mine ;) Its almost done,
just a few minor issues remaining.
But, I'm unable to understand why you're using `!currentIndex.isValid()` in if
(distance lastDistance || !currentIndex.isValid()) ?
Why do we need to validate the currentIndex ?
- Shantanu
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-03-20 22:14:51, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-03-20 22:14:51)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1255
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1255
The implementation of this function suffers from the same problem as
the one above.
It should also use the smoothScroll() function instead of changing the
scrollbar value directly.
I would implement it by doing something like:
const QRect r = mapFromViewport(visualRect(index));
if (r.top() 0) {
smoothScroll(0, r.top());
} else if (r.bottom() geometry().height()) {
smoothScroll(0, r.bottom() - geometry().height());
}
I've reverted whole of this back, and currently will work on this problem of
user having rearranged the icons. Will submit a new patch when I'm done.
- Shantanu
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-03-20 22:14:51, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-03-20 22:14:51)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote: /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208 http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208 A problem with the way this function is implemented is that it assumes that the view is always sorted and that the icons always flow from left to right. When the user has rearranged the icons (m_layoutBroken is true), you have to assume that the icons are no longer arranged in a grid and that the visual order no longer matches the order in the model. When this is the case, and the user has pressed the up key for example, you have to iterate over all the icons and find the one that is closest to the current icon while still being above it. you have to iterate over all the icons. I'm working on this by iterating all icons and finding the nearest one to the current selection according to the key pressed, but the code is getting really complex in terms of calculations. I was wondering if there is any other way of doing this? If anyone has an idea, please let me know. Till then I'm working on it. - Shantanu --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/#review541 --- On 2009-03-20 22:14:51, Shantanu Tushar Jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-03-20 22:14:51) Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
On 2009-03-20 14:07:32, Fredrik Höglund wrote:
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
http://reviewboard.kde.org/r/368/diff/2/?file=3392#file3392line1208
A problem with the way this function is implemented is that it assumes
that the view is always sorted and that the icons always flow from left to
right.
When the user has rearranged the icons (m_layoutBroken is true), you
have to assume that the icons are no longer arranged in a grid and that the
visual order no longer matches the order in the model.
When this is the case, and the user has pressed the up key for example,
you have to iterate over all the icons and find the one that is closest to
the current icon while still being above it.
wrote:
you have to iterate over all the icons. I'm working on this by
iterating all icons and finding the nearest one to the current selection
according to the key pressed, but the code is getting really complex in terms
of calculations. I was wondering if there is any other way of doing this? If
anyone has an idea, please let me know. Till then I'm working on it.
I would do something like this (the example is for the up key only):
QModelIndex nextIndex = QModelIndex();
QPoint currentPos = visualRect(currentIndex).center();
int lastDistance = 0;
for (int i = 0; i m_validRows; i++) {
const QModelIndex index = m_model-index(i, 0);
const QPoint pos = visualRect(index).center();
if (pos.y() currentPos.y()) {
int distance = (pos - currentPos).manhattanLength();
if (distance lastDistance || !currentIndex.isValid()) {
nextIndex = index;
lastDistance = distance;
}
}
}
If nextIndex is valid when you get here, it's the index you should move to.
If it isn't valid there are no icons above the current icon.
Thanks for working on this feature :)
- Fredrik
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---
On 2009-03-20 22:14:51, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-03-20 22:14:51)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- Review request for Plasma. Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 941694 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 941694 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review537
---
this is a very important feature to add, so thanks for working on it. there are
a number of small things that need to be improved a bit in the patch before it
can go in, but you've done most of the hard work already! :)
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment321
code style: no spaces around or inside the ()s, and you don't need to use
this-. just: setFocusPolicy(Qt::StrongFocus);
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment322
code style: one line per variable:
int hdirection = 0;
int vdirection = 0;
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment323
switch (event-key()) {
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment324
coding style: an if should be written this way:
if (..conditional..) {
... block ...
}
braces are required even on single line if/while/for statements
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment325
this could be written as:
case Qt::Key_Return:
case Qt::Key_Enter:
emit activated(m_selectionModel-currentIndex();
break;
C++ supports fall throughs in switch statements that make this nice.
also, if the key pressed was Return or Enter, there is no need to do any
repainting or movement of the selection index. so the method should simply
return at this point.
finally, there should be a default case at the end to avoid compiler
warnings:
default:
break;
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment326
this seems like more painting than necessary? unless the scrollbar moves,
only the two icons that changed (the one that was selected and the one that is
now selected) need to be repainted.
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment327
this should be an if/else if/else statement:
if (currentRow == numberOfRows) {
m_scrollBar-setValue(m_scrollBar-maximum());
}
else if( currentRow == 1 ) {
m_scrollBar-setValue(m_scrollBar-minimum());
} else {
m_scrollBar-setValue(division * currentRow);
}
otherwise it will never set the scrollbar to the minimum when currentRow ==
1, but to division. this should solve the problem you were having.
- Aaron
On 2009-03-20 09:14:10, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-03-20 09:14:10)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 941694
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 941694
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-03-20 10:21:32.420468) Review request for Plasma. Changes --- Aaron, I couldn't believe I actually used a this- there, and it was so silly of me to forget the else. Thanks for pointing out this and the coding styles. The diff is updated accordingly, and the scrollbar behavior is fine now. :) Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 941694 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 941694 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review540
---
logic looks alright, just a couple of code style issues to fix and then you can
commit this to svn. note that we use the kdelibs coding style in plasma. the
style is documented here: http://techbase.kde.org/Policies/Kdelibs_Coding_Style
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment328
please check the spacing in all your if statements: if (foo) {, not if( foo
) {.
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp
http://reviewboard.kde.org/r/368/#comment329
code style:
if (foo) {
} else if (foo) {
} else {
}
- Aaron
On 2009-03-20 10:21:32, Shantanu Tushar Jha wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---
(Updated 2009-03-20 10:21:32)
Review request for Plasma.
Summary
---
This partly addresses the above bug, adding keyboard navigation and launch
using Enter key.
Please report if the code is too complex, I've tried my best to keep it to
the point.
This addresses bug 187241.
https://bugs.kde.org/show_bug.cgi?id=187241
Diffs
-
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 941694
/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 941694
Diff: http://reviewboard.kde.org/r/368/diff
Testing
---
Tested on latest SVN build. Navigation and launch work fine. The problem is
with movement of the scrollbar with the keyboard focus, the scrollbar refuses
to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() );
is used. What am I doing wrong?
Thanks,
Shantanu
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add keyboard navigation to plasma applet Folder View
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/368/ --- (Updated 2009-03-20 22:14:51.976726) Review request for Plasma. Changes --- Have attached new diff. I've renamed the function to scrollTo(), with effort to adhere to the coding style. I will work on the following - 1. I'm not sure currently what/how to implement the parameter QAbstractItemView::ScrollHint hint, will work on this. 2. I've noted the problems of the view being unsorted, with IconView::keyPressEvent(QKeyEvent *event) and IconView::scrollTo(const QModelIndex index, QAbstractItemView::ScrollHint hint). I will work on them and try to resolve the problem. I'm not aware if I should commit in small working steps, or at once in the end. As the applet is in a working stage, is it ok to commit it? ( I commited a previous diff2, and after that I thought of the above. Regrets if I did it wrong :( ) Summary --- This partly addresses the above bug, adding keyboard navigation and launch using Enter key. Please report if the code is too complex, I've tried my best to keep it to the point. This addresses bug 187241. https://bugs.kde.org/show_bug.cgi?id=187241 Diffs (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h 942106 /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp 942106 Diff: http://reviewboard.kde.org/r/368/diff Testing --- Tested on latest SVN build. Navigation and launch work fine. The problem is with movement of the scrollbar with the keyboard focus, the scrollbar refuses to go to minimum value when m_scrollBar-setValue( m_scrollBar-minimum() ); is used. What am I doing wrong? Thanks, Shantanu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
