Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

---
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


Don't forget to remove this before committing ;)



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


Too much indentation here.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


Please add a comment here saying that the fall-through is intentional. 



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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


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

[Shantanu Tushar Jha]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/
---

(Updated 2009-04-08 21:33:17.329268)


Review request for Plasma.


Changes
---

1.Indentation changed from 2 spaces to 4 spaces.
2.ScrollTo function moved to AbstractItemView class.
3.setFocusPolicy(Qt::StrongFocus) redundancy removed. Now it exists only in the 
constructor.
4.Attempt to resolve the issue with m_columns being 0
5.Modified code to work with all existing flow values - LeftToRight, 
TopToBottom, RightToLeft, TopToBottomRightToLeft.

Please review and comment if something is 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/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

[Shantanu Tushar Jha]

> 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
> > 
> >
> > 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
> > 
> >
> > 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

[Fredrik Höglund]

> 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
> > 
> >
> > 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
> > 
> >
> > 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

[Shantanu Tushar Jha]

> 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.

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.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h, line 119
> > 
> >
> > I'd prefer it if this function was in AbstractItemView instead, since 
> > the code will work for the ListView class as well.

Ok, done. Will reflect it in next Diff after I resolve the flow issue.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 110
> > 
> >
> > IconView already sets the focus policy to StrongFocus at the top of the 
> > constructor (as of today).
> >

Oh, I think it has been added to the constructor recently. I've removed my line.


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1344
> > 
> >
> > This could result in a division by zero.

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?


> On 2009-04-02 13:56:47, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1372
> > 
> >
> > 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.
> >

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 :)


- 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

[Fredrik Höglund]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

[Fredrik Höglund]

---
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


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


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


This could result in a division by zero.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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

[Shantanu Tushar Jha]

---
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

[Shantanu Tushar Jha]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

[Fredrik Höglund]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

[Shantanu Tushar Jha]

---
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

[Shantanu Tushar Jha]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

[Fredrik Höglund]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Shantanu Tushar Jha]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1208
> > 
> >
> > 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

[Shantanu Tushar Jha]

> On 2009-03-20 14:07:32, Fredrik Höglund wrote:
> > /trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp, line 1255
> > 
> >
> > 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

[Shantanu Tushar Jha]

---
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

Re: Review Request: Add keyboard navigation to plasma applet Folder View

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/368/#review541
---


I have some comments in addition to the coding style issues mentioned by Aaron.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.h


This function should be named scrollTo() to match the QAbstractItemView API.
In my opinion it also belongs in the AbstractItemView class, since it's not 
view specific.




/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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.




/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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());
}



- Fredrik


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

[Aaron Seigo]

---
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


please check the spacing in all your if statements: if (foo) {, not if( foo 
) {.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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

[Shantanu Tushar Jha]

---
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

[Aaron Seigo]

---
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


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


code style: one line per variable:

int hdirection = 0;
int vdirection = 0;



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


switch (event->key()) {



/trunk/KDE/kdebase/apps/plasma/applets/folderview/iconview.cpp


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


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


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


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

Review Request: Add keyboard navigation to plasma applet Folder View

[Shantanu Tushar Jha]

---
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