Re: Polishing sprint in May: dates?
Hello, While not strictly on-topic, may I use the chance and ask if there is any probability of Plasma or KDE in general having a sprint somewhere in Northern Europe, i.e. Germany, Finland, Sweden? At the very least, in the new office in Edinburgh? Thank you and apologies for the off-topic question! Best regards, Ignat Semenov Sebastian Kügler wrote: > Hey, > > A bunch of us want to get together in May to put finishing touches in > Plasma Next. This sprint is important because it allows us to sit down and > fix remaining bugs. > > I am proposing dates here, though this is meant as a "soft proposal", May > is a busy month, and we can't realistically get everyone into the same > place. But even just with a few of us, it will be worth it. We will meet > in the Blue Systems Barcelona office, if possible for a week of frantic > bugfixing. > > I'd like to ask the KDE e.V. for funding of some of the costs, and some > people might need visa, so let's start early, and get the planning done > asap. We might not be able to get the e.V. to fund some of the costs, > since we already had a sprint in January, but let's try nevertheless. > > Please specify the dates that you could make to Barcelona until Friday > evening, latest, so we can proceed with the planning. > > http://doodle.com/4qygezcx9zfnvahb > > Cheers, ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
Hello guys, The branch has been merged to master with a few more cleanups (namely fixed missing or incorrect "What's this" for the newly added items; changed the page name to "Icons"; changed the page icon to "preferences-desktop-icons"). Thanks goes to Sho for reviewing the branch and nitpicking on everything, as well as general support. :) Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
OK forget that, just talked to aseigo and he told me there still is a usecase (and probably a few of those) for manual arrangement in the standalone mode. ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
Done, Mr Eagle Eye ;) OK I also need to add a menu to the list view, right? Currently there is no way to change sorting via the context menu. Arrangement and alignment do not make sense in the list view context, but sorting does. One more thing: In the standalone applet mode, but with an iconview (yes some people do use that ;) ) it makes no sense to configure the icon arrangement, alignment, align to grid, lock in place - I guess users are not expected to drag icons around to their liking in the standalone applets - those are like small file managers. (yes I know it's not a file namager and is not meant to be one, but you get the idea.) So all we're left with is Sorting - Criteria, Descending, Folders First. Thus the usual Icons -> Sort By -> bla bla is unnecessary since "Sort By" has 0 siblings. Thus maybe move "Sort By" in place of "Icons" when it's a standalone applet? That said, when I factor out the actions code, this is something to consider. Thank you! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
Done! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
Hello, git.k.o is finally up again, so here we go: - Split the flow enum in two - a mega-patch with a mega-commit-message - Re-did the UI with 4 categories Concerns: setLayout() and setAlignment() both trigger a complete relayout of the view. Relayouting the view twice is suboptimal, however, in the code there is a 10ms timer to delay the relayout so as not to block the caller. Should that timer be enough to ensure that when those two funcitons are called one after another, the view is only relayouted once? Maybe increase the delay a bit? Other than that, the split has made the code way more readable, in my opinion, not to say the it is now easier to configure the layout (I have had troubles wrapping my head around the top to bottom right to left stuff as well). Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
A UI draft: http://imagebin.org/254401 Maybe move "Icon size" to the "Arrangement" section? Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
Hello, Wow that was fast :) Glad to see you coding for plasma, your experience is very welcome (not to say I finally have someone to poke to death with design questions ;) ) - Display options is indeed a mess, well is it ok to mix combos and checkboxes together? i.e. 2 combos, 1 checkobox, then 1 combo and 2 checkboes etc. And they keep growing in number, although the existing changes should have added the last ones. - Icon alignment aka flow: I have been thinking about splitting that up ever since I saw it, but that means one more checkbox. On the other hand, honestly, it would make sense to split that in the code as well (see the current code - imho it sucks - to answer "are we using rows or columns" the flow is is compared with two enums, same for left vs right, not as easy to read). The UI, on the other hand, is up to aseigo, from what I understand - I'm all for it, but a decent UI advice is of course welcome. (fredrikh, what do you think?) - setCurrentItem() - which one did you like better, monolithic setCurrentItem(), find() and then set() depending on find(), or the last one with set() being unnecessary? For me, it's usually "code what to do not how", but this time I was not sure if it was just too much auxiliary code. - int(FolderViev::Unsorted) is a hack around the fact the sorting column is not homogenuous. 4 of them are enum KDirModel::(something), 1 is FV::Unsorted. Hence there is no way to have a real typed enum bracing them all in there. All the others, which you should have noticed, are enums with qvariant's since that gives you compiler warnings and errors in case you mess up - a nice thing imho :) Thank you for the advice! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Menu branch
A small update: I think that the setCurrentItem() abstraction is superfluous, not to mention that the name coincides with that of a QComboBox member function, thus I've put the workaround in findComboboxItem(), so now it is clear what is worked around, and once qt5 comes, it will be easy to swap the necessary code for QComboBox::findData() (in qt5, QVariant::operator==() works fine wrt to enums that are Q_DECLARE_METATYPE'd - tested by tsdgeos in a qt5 installation). For the record: [Thursday 11 April 2013] [00:07:23]qDebug() << (QVariant::fromValue(Test::two) == QVariant::fromValue(Test::two)); [Thursday 11 April 2013] [00:07:24]false [Thursday 11 April 2013] [00:08:30]Compares this QVariant with v and returns true if they are equal; otherwise returns false. [Thursday 11 April 2013] [00:08:32]In the case of custom types, their equalness operators are not called. Instead the values' addresses are compared. [Thursday 11 April 2013] [00:14:58]isemenov: yeah that works in qt5 [Thursday 11 April 2013] [00:15:05]qDebug() << (QVariant::fromValue(Test::two) == QVariant::fromValue(Test::two)); [Thursday 11 April 2013] [00:15:06]returns true Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Menu branch
Hello plasma devs and users, Please review the plasma/isemenov/menu branch. Based on #306537 but goes a bit further than that. Fixes: "Arrange icons" (flow configuration) added to the context menu "Sort order" and "folders first" added to the config UI Update the "Sort" combo if the user moves the icons while the dialog is open - fix by Eike Hein, I expressed it using a helper func since the relevant code has changed slightly Update sort order, sort direction, flow combos when the user cnahges those using the context menu if the config dialog is open Update the align to grid, lock in place, folders first, click to view checkboxes when the user cnahges those using the context menu if the config dialog is open Fix a potential stall (instead of iterating the combos from 0 till maxCount() (INTMAX) which can result in a minute-long loop (tested) if the value is not found iterate them from 0 till count()) - thanks goes to tsdgeos for explaiing me that comboboxes are contiguous Changed the hardcoded "-1" to an anonymous enum "FolderView::Unsorted = -1" for code clarity and maintainability Only show the "Unsorted" action in the "Sort" combo if the view is not sorted. It appears when the users drags icons around, and lives until the dialog is closed. Then, when the dialog is opened again, if the view is still unsorted, it is added to the list. If the user sorts the view using the combo or the context menu, unsorted still stays until the dialog is closed - probably better than have it appear and disappear within an open dialog. This implements aseigo's idea. Also, since I had to move the flow and sorting direction to QActionGroups (to enable them in the context menu), and that lead to duplicated for loops, I created a little helper to add action groups to combos. (trying to follow "code what to do, not how to do it"). Another thing worth mentioning is the setCurrentItem() helper func. I should be using QComboBox::findData(), *but* we use enums for the action data, hence QVariant's and Q_DECLARE_METATYPE. QVariant::operator==() fails for user defined types, including Q_DECLARE_METATYPE'd enums. Debugged together with tsdgeos and steveire. So we have to workaround that, in a manually written loop, which I put in a helper func. Thank you for your attention! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Folderview: add "Unsorted" to the "sort Icons" context menu?
Bump? Any considerations / opinions / ideas? Thank you! Best regards, Ignat Semenov On Fri, Mar 29, 2013 at 11:55 PM, Ignat Semenov wrote: > Hello guys! > > I've been polishing the context menu and Display page lately, both the > looks > and the code, and discovered this: the "Unsorted" option is only present in > the Display page combobox, but is missing from the sorting group in the > context menu. > > 1)Why do we need the unsorted option at all in the Display page combo? Are > the > user supposed to first set unsorted, then drag icons? From what I > understand > they just drag the icons, unsorted is just a consequence. > > 2)If it is required, then why is it present only in the config page combo? > What > do you think about adding that a the first option in the sorting context > menu? > > Thank you! > > Best regards, > Ignat Semenov > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Folderview: add "Unsorted" to the "sort Icons" context menu?
Hello guys! I've been polishing the context menu and Display page lately, both the looks and the code, and discovered this: the "Unsorted" option is only present in the Display page combobox, but is missing from the sorting group in the context menu. 1)Why do we need the unsorted option at all in the Display page combo? Are the user supposed to first set unsorted, then drag icons? From what I understand they just drag the icons, unsorted is just a consequence. 2)If it is required, then why is it present only in the config page combo? What do you think about adding that a the first option in the sorting context menu? Thank you! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Please delete 2 branches
Hello guys! I'd like an admin to delete the branches: remotes/origin/fvport remotes/origin/plasma/isemenov/folderview-qml The first one is a mistake, the second one has stuck and we decided to start a new one instead, ditching the old one. Thank you! Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
A folderview QGraphicsView related bug
Hello fellow plasma devs and users!
Let me apologise for the long break, now I'm back to some intense bug fixing.
Currently I'm investigating this bug in folderview:
Sometimes folder peek popups appear at (0,0) (upper left corner of the screen)
instead of a position depending on the relevant icon. Quickly narrowed the
issue down to IconView::openPopup (iconview.cpp:2672). First, it computes the
popup point relative to the icon view then it tries to find a QGraphicsView to
map the point to. The bug occurs when the second branch of
if (m_popupCausedWidget)
is executed, i.e. when m_popupCausedWidget is false (if we're not dragging, it
is false). In that second branch, all the views in the scene are iterated to
find the view under mouse:
foreach (QGraphicsView *view, scene()->views()) {
qDebug() << "view"' << view;
if (view->underMouse()) {
gv = view;
break;
}
Now according to the docs for QGraphicsView::underMouse(), it returns an
invalid value only if we're dragging, which is handled in the first branch. In
practice, sometimes, none of the views returns true, and gv remains 0, which
leads to the point being calculated as (0,0):
const QPoint pos = gv ? gv->mapToGlobal(gv->mapFromScene(scenePos)) : QPoint();
The debug output reads:
view QGraphicsView(0x19b2ae0)
view QGraphicsView(0x2193580)
view QGraphicsView(0x1fd88b0)
view PanelView(0x21c6dc0)
view DesktopView(0x1837010)
gv DesktopView(0x1837010)
These are the views being checked in openPopup(). Sometimes, when the bug
occurs, none of those returns true in underMouse().
Now the questions are:
1)Why do we need to iterate through the views like this? Anything more elegant
in the Plasma library?
2)Any ideas when underMouse() might fail like this when not dragging? Seems to
be completely random.
Basically, we need to change the iteration loop to something more reliable, and
that will fix the bug.
Best regards,
Ignat Semenov
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Hide action buttons at certain icon sizes
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104413/ --- Review request for Plasma, Aaron J. Seigo and Fredrik Höglund. Description --- This patch disables action icons if they cover the actual icon. Thus, tthe buttons being enabled depends on the grid size, the icon size and the svg margin size. I've tried a lot of different approaches in order to write the shortest and most transparent code, hope the current one is fine from this perspective. Please pay attention to design and incapsulation issues, I'm not a good designer at all. This addresses bug 268641. http://bugs.kde.org/show_bug.cgi?id=268641 Diffs - plasma/applets/folderview/actionoverlay.h e7c1982 plasma/applets/folderview/actionoverlay.cpp 4dd1975 plasma/applets/folderview/iconview.h c76b4f4 plasma/applets/folderview/iconview.cpp 56f1eb8 Diff: http://git.reviewboard.kde.org/r/104413/diff/ Testing --- Tested, works. I still wonder, however, why we can't have a checkbox to disable the dreaded "+" button.. although the patch submitted in thei review request should go in regardless of such a checkbox being added because at some icon sizes, the action icons are a plain annoyance, indeed. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Scroll automatically when touching the border with the rubberband
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104396/ --- (Updated March 24, 2012, 12:42 p.m.) Review request for Plasma, Aaron J. Seigo and Fredrik Höglund. Changes --- Stop autoscrolling on lmb release with an active rubberband. Description --- Wehn dragging a rubberband over an icon view with a scrollbar, scroll the view automatically on touching the border with the mouse during drag. I've had to mod stopAutoScrolling() and call it in IconView::wheelEvent() and AbstractItemView::scrollBarActionTriggered() to allow the user to stop the automatic scrolling on those events. This addresses bug 189350. http://bugs.kde.org/show_bug.cgi?id=189350 Diffs (updated) - plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/iconview.h 0bd6dc0 plasma/applets/folderview/iconview.cpp 36e640b Diff: http://git.reviewboard.kde.org/r/104396/diff/ Testing --- Tested, works. Maybe we should also stop automatic scrolling if the user releases the LMB while scrollling automatically as a result of dragging the rubberband over the view border? Fredrik, what do you think? Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Scroll automatically when touching the border with the rubberband
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104396/ --- Review request for Plasma, Aaron J. Seigo and Fredrik Höglund. Description --- Wehn dragging a rubberband over an icon view with a scrollbar, scroll the view automatically on touching the border with the mouse during drag. I've had to mod stopAutoScrolling() and call it in IconView::wheelEvent() and AbstractItemView::scrollBarActionTriggered() to allow the user to stop the automatic scrolling on those events. This addresses bug 189350. http://bugs.kde.org/show_bug.cgi?id=189350 Diffs - plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/iconview.h 0bd6dc0 plasma/applets/folderview/iconview.cpp 36e640b Diff: http://git.reviewboard.kde.org/r/104396/diff/ Testing --- Tested, works. Maybe we should also stop automatic scrolling if the user releases the LMB while scrollling automatically as a result of dragging the rubberband over the view border? Fredrik, what do you think? Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Less repainting on mousePressEvent(), moseReleaseEvent() and mouseDoubleClickEvent() in FolderView::IconView
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103822/ --- (Updated March 22, 2012, 1:07 p.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Changes --- Fixed a one-line issue where multiple selection would not be repainted with single-click settings. In mouseReleaseEvent(), in the single click branch, we should not reset the selection as this will result in the code in the following if clause not being executed and the selection not repainted. Now this is ready to go. Sorry for the delay, I'm cleaning up my old requests now, finally. Description --- This patch aims to save some repaints in FolderView::IconView on the various mouseEvent()'s by choosing what to repaint in a bit smarter way. Diffs (updated) - plasma/applets/folderview/iconview.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 Diff: http://git.reviewboard.kde.org/r/103822/diff/ Testing --- Testing done against master, seems to behave indentically. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add a link to the current desktop folder instead of an icon applet if the desktop is set to folderview on the "Add to Desktop" Kickoff action
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103830/ --- (Updated March 20, 2012, 5:05 p.m.) Review request for Plasma and Aaron J. Seigo. Changes --- addUrl(KUrl) -> addUrls(const KUrl::List&) ReviewBoard and multiple git repos for different parts of kde is not the most comfortable thing to use, though.. Description --- Currently, right-clicking a Kickoff/Classical application launcher entry and selectiong "Add to Desktop" puts an icon applet on the desktop. However, if the desktop is set to Folderview, it is more consistent to add a link to the currently displayed folder. This patch cheks if the "folderview" plugin is used in the desktop containment and performs a KIO::link() if that's the case. Diffs (updated) - plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 99c2649 Diff: http://git.reviewboard.kde.org/r/103830/diff/ Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: add public slot FolderView::addUrl(KUrl)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104328/ --- (Updated March 20, 2012, 5:03 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- addUrl(KUrl) -> addUrls(const KUrl::List&) Description --- add public slot FolderView::addUrl(KUrl) This slot can be used e.g. by Kickoff for the "Add to Desktop" action to add a real link instead of an Icon applet. Diffs (updated) - plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp 121d1e7 Diff: http://git.reviewboard.kde.org/r/104328/diff/ Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: add public slot FolderView::addUrl(KUrl)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104328/ --- Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- add public slot FolderView::addUrl(KUrl) This slot can be used e.g. by Kickoff for the "Add to Desktop" action to add a real link instead of an Icon applet. Diffs - plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp 121d1e7 Diff: http://git.reviewboard.kde.org/r/104328/diff/ Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add a link to the current desktop folder instead of an icon applet if the desktop is set to folderview on the "Add to Desktop" Kickoff action
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103830/ --- (Updated March 18, 2012, 6:38 a.m.) Review request for Plasma and Aaron J. Seigo. Changes --- use QMetaObject::indexOfSlot to check for addUrl in the target containment This is useful when e.g. the target containmnet is a FolderView, and we need to add a link instead of an Icon applet on triggering the "Add to Desktop" action. Description --- Currently, right-clicking a Kickoff/Classical application launcher entry and selectiong "Add to Desktop" puts an icon applet on the desktop. However, if the desktop is set to Folderview, it is more consistent to add a link to the currently displayed folder. This patch cheks if the "folderview" plugin is used in the desktop containment and performs a KIO::link() if that's the case. Diffs (updated) - plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 8db9655 Diff: http://git.reviewboard.kde.org/r/103830/diff/ Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: do not crash when passing a url in the ctor e.g. when creating a folderview by dropping a folder onto the desktop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104311/ --- (Updated March 17, 2012, 9:52 a.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- Do not call setUrl in the ctor since m_dirLister does not exist yet. Instead, assign m_url in the ctor and call setUrl later in init(). Diffs - plasma/applets/folderview/folderview.cpp a94ce87 Diff: http://git.reviewboard.kde.org/r/104311/diff/ Testing --- Tested, works. Would be nice to give the code a static analyzer run, if there was a decent static analyzer available in Linux. Anybody have relevant experience? Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: do not crash when passing a url in the ctor e.g. when creating a folderview by dropping a folder onto the desktop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104311/ --- (Updated March 17, 2012, 9:52 a.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- Do not call setUrl in the ctor since m_dirLister does not exist yet. Instead, assign m_url in the ctor and call setUrl later in init(). Diffs - plasma/applets/folderview/folderview.cpp a94ce87 Diff: http://git.reviewboard.kde.org/r/104311/diff/ Testing --- Tested, works. Would be nice to give the code a static analyzer run, if there was a decent static analyzer available in Linux. Anybody have relevant experience? Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: do not crash when passing a url in the ctor e.g. when creating a folderview by dropping a folder onto the desktop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104311/ --- Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- Do not call setUrl in the ctor since m_dirLister does not exist yet. Instead, assign m_url in the ctor and call setUrl later in init(). Diffs - plasma/applets/folderview/folderview.cpp a94ce87 Diff: http://git.reviewboard.kde.org/r/104311/diff/ Testing --- Tested, works. Would be nice to give the code a static analyzer run, if there was a decent static analyzer available in Linux. Anybody have relevant experience? Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
> On March 13, 2012, 1:12 p.m., Marco Martin wrote: > > looks good, a thing i would like to be tested is when the saved position is > > invalid, like either negative or an enormous value. > > > > this shouldn't break it (is even quite probable a value not being valid > > anymore because there are less files than the previous session) > > Fredrik Höglund wrote: > I think it would be a good idea to save the modification time for the > folder and use that to check if the saved scrollbar value is likely to be > invalid. If the user is able to scroll the view while the layout is in > progress, this should also abort restoring the position. > > Also, I'm wondering if we really need to save the position separately for > the iconview and the listview, or if we should use the same key. > > Otherwise the patch looks good to me. > > Ignat Semenov wrote: > Well I've been thinking about separate vs single key and I think separate > is easier to read and maintain, less checks and branches. > > The main problem with a single key would be when the applet is put into a > panel, first the icon view gets created and grabs the value, then the list > view gets created and gets a 0. Two keys are easier to work with I think. > > As for scrolling when the layout is in progress, this method is intended > to be used at startup only, so the user can not scroll the view. Or do you > mean that some dev could use restoreScrollbarPosition() manually after > startup? > > Folder mtime is a nice idea, one more corner case, will try to implement. > > Ignat Semenov wrote: > Actually, aborting the automatic scrolling works just fine, as > smoothScroll(savedPosition) is used and that one can be interrupted easily. > > Ignat Semenov wrote: > OK, as discussed with fredrikh on IRC yesterday, the patch now accounts > for multiple layout passes. > As far as the dir mtime issue goes, I think that actually falls into the > range check case, that is, if the dir content changes, but the number of rows > does not, it's probably ok to restore the scroll position. If the number of > rows changes, then the scrollbar range changes and that is caught by the > check in scrollToSavedPosition(). Same for the list view. > > Now the only relevant issue is actually aborting the restore if scrolling > the view between layout passes in a slow dir. OK, the only thing left now is to correctly handle this: applet created on the desktop -> scrolled -> put into the panel ->url changed -> dragged back to the desktop -> scrolled again, but the dir is different and the scroll is invalid on the other hand, we can't simply discard the value after restoring the position, since if the above is done without changing the url, the scrollbar restore on panel -> desktop is desired and correct. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- On March 16, 2012, 4:31 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104258/ > --- > > (Updated March 16, 2012, 4:31 p.m.) > > > Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. > > > Description > --- > > This patch implements scrolbar position saving on plasma exit. The change is > fairly trivial, however, due to the fact that the view is not populated and > layouted immediately simply scrolling to the desired position on creating the > view does not work. Instead a signal is emitted on finishing the item layout, > when the view has a valid size and the scrollbar has a valid range. The > signal is connected to a slot which scrolls the view to the desired position > and then disconnects the signal. For the user, a public function in > AbstractItemView is introduced, which performs the connection. > > The only problem is that ListView turned out not to have any layout method. > It just paints the items one by one, calculating their position on the fly, > so I put the signal at the end of updateScrollbar to ensure the scrollbar > range is valid. Maybe it should go into the "if (max>0)" branch? > > > This addresses bug 261139. > http://bugs.kde.org/show_bug.cgi?id=261139 > > > Diffs > - > > plasma/applets/folderview/abstractitemview.h aa68b90 >
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 4:31 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Avoid race condition between layoutFinished() and restoreScrollBarPosition(). Connect restoreScrollBarPosition() and scrollToSavedPosition() in the AbstractItemView ctor. Introduce AbstractItemView::setSavedScrollbarPosition(int) and call that before setModel in teh view setup methods in FolderView. Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 1:47 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Call discardScrollbarRestore() directly from AbstractItemView::scrollBarActionTriggered(). Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 16, 2012, 10:02 a.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Discard the scrollbar position restore if the user has manually scrolled the view before the listing is finished. The patch is for IconView only. There is a big problem with doing the same in ListView, actuallly, it is unnecessary there at all. This is why: There is no multi-pass layout in ListView; moreover, when the user clicks the icon in the panel, and the listing starts, the popup won't open before the listing is finished. The user has no chance of scrolling the view before the listing is finished, so the problem doens't even exist there. What botheres me is the Plasma stall on loading a big dir. I think we should port the multiple-pass layout code to the ListView class to ensure responsiveness, but that will be a different review request. Fredrik, what do you think? This RR is finished now, I think it's ready to go. Please, do nitpick :) Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp 94efe44 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
> On March 13, 2012, 1:12 p.m., Marco Martin wrote: > > looks good, a thing i would like to be tested is when the saved position is > > invalid, like either negative or an enormous value. > > > > this shouldn't break it (is even quite probable a value not being valid > > anymore because there are less files than the previous session) > > Fredrik Höglund wrote: > I think it would be a good idea to save the modification time for the > folder and use that to check if the saved scrollbar value is likely to be > invalid. If the user is able to scroll the view while the layout is in > progress, this should also abort restoring the position. > > Also, I'm wondering if we really need to save the position separately for > the iconview and the listview, or if we should use the same key. > > Otherwise the patch looks good to me. > > Ignat Semenov wrote: > Well I've been thinking about separate vs single key and I think separate > is easier to read and maintain, less checks and branches. > > The main problem with a single key would be when the applet is put into a > panel, first the icon view gets created and grabs the value, then the list > view gets created and gets a 0. Two keys are easier to work with I think. > > As for scrolling when the layout is in progress, this method is intended > to be used at startup only, so the user can not scroll the view. Or do you > mean that some dev could use restoreScrollbarPosition() manually after > startup? > > Folder mtime is a nice idea, one more corner case, will try to implement. > > Ignat Semenov wrote: > Actually, aborting the automatic scrolling works just fine, as > smoothScroll(savedPosition) is used and that one can be interrupted easily. OK, as discussed with fredrikh on IRC yesterday, the patch now accounts for multiple layout passes. As far as the dir mtime issue goes, I think that actually falls into the range check case, that is, if the dir content changes, but the number of rows does not, it's probably ok to restore the scroll position. If the number of rows changes, then the scrollbar range changes and that is caught by the check in scrollToSavedPosition(). Same for the list view. Now the only relevant issue is actually aborting the restore if scrolling the view between layout passes in a slow dir. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- On March 13, 2012, 6:44 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104258/ > --- > > (Updated March 13, 2012, 6:44 p.m.) > > > Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. > > > Description > --- > > This patch implements scrolbar position saving on plasma exit. The change is > fairly trivial, however, due to the fact that the view is not populated and > layouted immediately simply scrolling to the desired position on creating the > view does not work. Instead a signal is emitted on finishing the item layout, > when the view has a valid size and the scrollbar has a valid range. The > signal is connected to a slot which scrolls the view to the desired position > and then disconnects the signal. For the user, a public function in > AbstractItemView is introduced, which performs the connection. > > The only problem is that ListView turned out not to have any layout method. > It just paints the items one by one, calculating their position on the fly, > so I put the signal at the end of updateScrollbar to ensure the scrollbar > range is valid. Maybe it should go into the "if (max>0)" branch? > > > This addresses bug 261139. > http://bugs.kde.org/show_bug.cgi?id=261139 > > > Diffs > - > > plasma/applets/folderview/abstractitemview.h aa68b90 > plasma/applets/folderview/abstractitemview.cpp 3debb70 > plasma/applets/folderview/folderview.h 4e441eb > plasma/applets/folderview/folderview.cpp a94ce87 > plasma/applets/folderview/iconview.h 12e93b3 > plasma/applets/folderview/iconview.cpp 5c4e086 > plasma/applets/folderview/listview.cpp b17e7c4 > > Diff: http://git.reviewboard.kde.org/r/104258/diff/ > > > Testing > --- > > Tested both the icon view and the list view, works fine. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 13, 2012, 6:44 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Ensure the layout is really finished before emitting layoutFinished(). Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.h 12e93b3 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp b17e7c4 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
> On March 13, 2012, 1:12 p.m., Marco Martin wrote: > > looks good, a thing i would like to be tested is when the saved position is > > invalid, like either negative or an enormous value. > > > > this shouldn't break it (is even quite probable a value not being valid > > anymore because there are less files than the previous session) > > Fredrik Höglund wrote: > I think it would be a good idea to save the modification time for the > folder and use that to check if the saved scrollbar value is likely to be > invalid. If the user is able to scroll the view while the layout is in > progress, this should also abort restoring the position. > > Also, I'm wondering if we really need to save the position separately for > the iconview and the listview, or if we should use the same key. > > Otherwise the patch looks good to me. > > Ignat Semenov wrote: > Well I've been thinking about separate vs single key and I think separate > is easier to read and maintain, less checks and branches. > > The main problem with a single key would be when the applet is put into a > panel, first the icon view gets created and grabs the value, then the list > view gets created and gets a 0. Two keys are easier to work with I think. > > As for scrolling when the layout is in progress, this method is intended > to be used at startup only, so the user can not scroll the view. Or do you > mean that some dev could use restoreScrollbarPosition() manually after > startup? > > Folder mtime is a nice idea, one more corner case, will try to implement. Actually, aborting the automatic scrolling works just fine, as smoothScroll(savedPosition) is used and that one can be interrupted easily. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 --- On March 13, 2012, 1:51 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104258/ > --- > > (Updated March 13, 2012, 1:51 p.m.) > > > Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. > > > Description > --- > > This patch implements scrolbar position saving on plasma exit. The change is > fairly trivial, however, due to the fact that the view is not populated and > layouted immediately simply scrolling to the desired position on creating the > view does not work. Instead a signal is emitted on finishing the item layout, > when the view has a valid size and the scrollbar has a valid range. The > signal is connected to a slot which scrolls the view to the desired position > and then disconnects the signal. For the user, a public function in > AbstractItemView is introduced, which performs the connection. > > The only problem is that ListView turned out not to have any layout method. > It just paints the items one by one, calculating their position on the fly, > so I put the signal at the end of updateScrollbar to ensure the scrollbar > range is valid. Maybe it should go into the "if (max>0)" branch? > > > This addresses bug 261139. > http://bugs.kde.org/show_bug.cgi?id=261139 > > > Diffs > - > > plasma/applets/folderview/abstractitemview.h aa68b90 > plasma/applets/folderview/abstractitemview.cpp 3debb70 > plasma/applets/folderview/folderview.h 4e441eb > plasma/applets/folderview/folderview.cpp a94ce87 > plasma/applets/folderview/iconview.cpp 5c4e086 > plasma/applets/folderview/listview.cpp b17e7c4 > > Diff: http://git.reviewboard.kde.org/r/104258/diff/ > > > Testing > --- > > Tested both the icon view and the list view, works fine. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
> On March 13, 2012, 1:12 p.m., Marco Martin wrote: > > looks good, a thing i would like to be tested is when the saved position is > > invalid, like either negative or an enormous value. > > > > this shouldn't break it (is even quite probable a value not being valid > > anymore because there are less files than the previous session) > > Fredrik Höglund wrote: > I think it would be a good idea to save the modification time for the > folder and use that to check if the saved scrollbar value is likely to be > invalid. If the user is able to scroll the view while the layout is in > progress, this should also abort restoring the position. > > Also, I'm wondering if we really need to save the position separately for > the iconview and the listview, or if we should use the same key. > > Otherwise the patch looks good to me. Well I've been thinking about separate vs single key and I think separate is easier to read and maintain, less checks and branches. The main problem with a single key would be when the applet is put into a panel, first the icon view gets created and grabs the value, then the list view gets created and gets a 0. Two keys are easier to work with I think. As for scrolling when the layout is in progress, this method is intended to be used at startup only, so the user can not scroll the view. Or do you mean that some dev could use restoreScrollbarPosition() manually after startup? Folder mtime is a nice idea, one more corner case, will try to implement. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/#review11354 ----------- On March 13, 2012, 1:51 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104258/ > --- > > (Updated March 13, 2012, 1:51 p.m.) > > > Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. > > > Description > --- > > This patch implements scrolbar position saving on plasma exit. The change is > fairly trivial, however, due to the fact that the view is not populated and > layouted immediately simply scrolling to the desired position on creating the > view does not work. Instead a signal is emitted on finishing the item layout, > when the view has a valid size and the scrollbar has a valid range. The > signal is connected to a slot which scrolls the view to the desired position > and then disconnects the signal. For the user, a public function in > AbstractItemView is introduced, which performs the connection. > > The only problem is that ListView turned out not to have any layout method. > It just paints the items one by one, calculating their position on the fly, > so I put the signal at the end of updateScrollbar to ensure the scrollbar > range is valid. Maybe it should go into the "if (max>0)" branch? > > > This addresses bug 261139. > http://bugs.kde.org/show_bug.cgi?id=261139 > > > Diffs > - > > plasma/applets/folderview/abstractitemview.h aa68b90 > plasma/applets/folderview/abstractitemview.cpp 3debb70 > plasma/applets/folderview/folderview.h 4e441eb > plasma/applets/folderview/folderview.cpp a94ce87 > plasma/applets/folderview/iconview.cpp 5c4e086 > plasma/applets/folderview/listview.cpp b17e7c4 > > Diff: http://git.reviewboard.kde.org/r/104258/diff/ > > > Testing > --- > > Tested both the icon view and the list view, works fine. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 13, 2012, 1:51 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Check the saved position for validity. Actually, I used to have that check, but removed it later because it was in restoreScrollbarPosition() and the range was of course invalid in that method. Silly me did not think about putting it where it belongs. :) Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs (updated) - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp b17e7c4 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- (Updated March 13, 2012, 1:06 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Changes --- Added bug number. Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? This addresses bug 261139. http://bugs.kde.org/show_bug.cgi?id=261139 Diffs - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp b17e7c4 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Save scrollbar position on plasma exit
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104258/ --- Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- This patch implements scrolbar position saving on plasma exit. The change is fairly trivial, however, due to the fact that the view is not populated and layouted immediately simply scrolling to the desired position on creating the view does not work. Instead a signal is emitted on finishing the item layout, when the view has a valid size and the scrollbar has a valid range. The signal is connected to a slot which scrolls the view to the desired position and then disconnects the signal. For the user, a public function in AbstractItemView is introduced, which performs the connection. The only problem is that ListView turned out not to have any layout method. It just paints the items one by one, calculating their position on the fly, so I put the signal at the end of updateScrollbar to ensure the scrollbar range is valid. Maybe it should go into the "if (max>0)" branch? Diffs - plasma/applets/folderview/abstractitemview.h aa68b90 plasma/applets/folderview/abstractitemview.cpp 3debb70 plasma/applets/folderview/folderview.h 4e441eb plasma/applets/folderview/folderview.cpp a94ce87 plasma/applets/folderview/iconview.cpp 5c4e086 plasma/applets/folderview/listview.cpp b17e7c4 Diff: http://git.reviewboard.kde.org/r/104258/diff/ Testing --- Tested both the icon view and the list view, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Workflow Idea for 4.10
Ben Cooksley wrote: > On Tue, Mar 13, 2012 at 7:58 PM, Ignat Semenov > wrote: >> Hello fellow KDE devs! >> >> While I'm not an experienced developer nor manager, the planned >> transition to gerrit really troubles me. In particular, I have the >> following questions: >> >> 1)The gerrit installation used in qt makes it impossible to add comments >> other than directly to the diff. No way to add comments on the main >> review request page as it is in the RB installation we're currently >> using. Is there any way to overcome that limitation? >> >> 2)The user interface of gerrit is horrible to say the least. The diff / >> comment area is the last class citizen there. RR is way more clear and >> user- friendly (esp. newcomers). >> >> 3)Does this transition mean we will have to use the full gerrit >> contribution cycle, like it is in qt now, with branches and the special >> tools, even for the smallest fixes? This will drive off new contributors, >> I'm afraid. > > Direct contributions by those people who have a KDE Developer account > will always be possible and will never be forced to go through Gerrit. > > Whilst code review is a nice thing, it is completely unnecessary with > trivial changes or minor bugfixes. How it is used with larger changes > is up to the projects themselves - but use of it will never be > compulsory - direct access will always be available. > OK, I see. Actually, there is a bit of a problem there ATM, in my opinion, in that very few projects seem to be using ReviewBoard. Plasma, KWin and Telepathy are the main users of RB, with some occasional kdelibs requests, from what I can see. I think we have a lot more projects being developed? :) >> >> 4)The Qt gerrit installation requires authentication just to browse the >> existing requests read-only. Is it possible to do it differently or is >> this a shortcoming of gerrit ( or its "design feature")? Pretty user- and >> newcomer-unfriendly, too. >> >> Best regards, >> Ignat Semenov >> ___ >> Plasma-devel mailing list >> Plasma-devel@kde.org >> https://mail.kde.org/mailman/listinfo/plasma-devel > > Regards, > Ben Cooksley > KDE Sysadmin Oh, and One More Thing (TM).It is impossible to add a detailed description to the change with the Qt installation of gerrit. You can only use the commit message for that, and commit messages, while in theory unlimited, still have some rules applied to them, and present only the gist of the change. What if I want to elaborate on the reasons of a particular design / code decision, or tell a story of the various approaches attempted, etc? It is impossibel to add any comments or text besides what is in the commit message. Given all those limitations of Gerrit (some of them may actually be the limitations of the Qt Gerrit installation), it looks like a clear downgrade form ReviewBoard to me. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Workflow Idea for 4.10
Hello fellow KDE devs! While I'm not an experienced developer nor manager, the planned transition to gerrit really troubles me. In particular, I have the following questions: 1)The gerrit installation used in qt makes it impossible to add comments other than directly to the diff. No way to add comments on the main review request page as it is in the RB installation we're currently using. Is there any way to overcome that limitation? 2)The user interface of gerrit is horrible to say the least. The diff / comment area is the last class citizen there. RR is way more clear and user- friendly (esp. newcomers). 3)Does this transition mean we will have to use the full gerrit contribution cycle, like it is in qt now, with branches and the special tools, even for the smallest fixes? This will drive off new contributors, I'm afraid. 4)The Qt gerrit installation requires authentication just to browse the existing requests read-only. Is it possible to do it differently or is this a shortcoming of gerrit ( or its "design feature")? Pretty user- and newcomer-unfriendly, too. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add a link to the current desktop folder instead of an icon applet if the desktop is set to folderview on the "Add to Desktop" Kickoff action
> On Feb. 1, 2012, 11:35 a.m., Aaron J. Seigo wrote: > > i really don't like this change because it puts in a hardcoded assumption > > about folderview into an independent component. is folderview the only > > applet/containment that should behave this way? probably not. what if we > > replace folderview someday with something else? is kickoff the only place > > from which you can add an item to a folderview? > > > > it's just not generic enough, and that "no assumptions" approach is > > something that we stick to in plasma which allows us to easily pull > > components out, replace them with others, etc. we already have this solved > > when there is a drag and drop event: each applet handles its own drop > > events and does what is necessary. > > > > on the other hand, i also don't want to see an "addUrl" type method to > > Applet since that is going to be irrelevant to the vast majority of Applets > > and itself contains some assumptions. > > > > this calls for a better solution that can be implemented in the target > > Applet and which doesn't result in assumptions about what the source or > > targets are. OK how about if we commit this for 4.8.1 and provide the 100% correct fix in master and 4.8.2? - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103830/#review10265 --- On Jan. 31, 2012, 3:45 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103830/ > --- > > (Updated Jan. 31, 2012, 3:45 p.m.) > > > Review request for Plasma and Aaron J. Seigo. > > > Description > --- > > Currently, right-clicking a Kickoff/Classical application launcher entry and > selectiong "Add to Desktop" puts an icon applet on the desktop. However, if > the desktop is set to Folderview, it is more consistent to add a link to the > currently displayed folder. This patch cheks if the "folderview" plugin is > used in the desktop containment and performs a KIO::link() if that's the case. > > > Diffs > - > > plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 8db9655 > > Diff: http://git.reviewboard.kde.org/r/103830/diff/ > > > Testing > --- > > Tested, works. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Unreproducible bug
Hello fellow plasma devs! There is a (well-known :D) bug here: https://bugs.kde.org/show_bug.cgi?id=294795 and I've only seen it once or twice a month ago. Since then, I haven't been able to reproduce it at all, and I feel that somebody, most probably aseigo, has fixed it indirectly in the meanwhile. So I'd like to ask you guys, especially Aaron:have you fixed any containment sizing related bugs recently? Anything in the desktop containment or deeper in libplasma? If not, I'll just dig into the FW code to try and figure stuff out.. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Folderview selection marker, revisited
Hello fello plasma devs! Some fo you may remember the "disable selection marker on d-click kde setting" feature I did in January. Back then, we settled on "don't add options just because we can". But recently, there has appeared one more comment on the bug, describing a separate problem which I ironically experienced yesterday as well. The problem is as follows: when the icon size is set to 16 or 22, it is impossible to hit the icon. With 16, it is absolutely inmpossible - you can test it yourself. At least with the Oxygen plasma theme (3px margins), though I'm not sure if it depends on the margins at all - need to check the code. So, the commenter asks for an option to disable the marker for this exact reason. At the smae time, he does not want to switch to d-click for solving this problem, and I understand him - it is at best a workaround. So what do you think we can do here? One option is to disable the marker altogether if the icon size vs marker size hits some limit, or the frame size vs marker size. Another one is to make the marker smaller based on a similar condition. The third one is, of coure, the option to manually disable the marker - but that would void my work on the automatic detection. Opinions? This may look like a minor issue not worth sucha long email and such a long discussion, but these minor nitpicks, or lack thereof, do sum up to crate a positive user experience. Best regards Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: constraints and size hints
Well, yes, my bad. My apologies. OK, so here are the issues: 1)I'd like to understand the relation between constraintsEvent() and sizeHint(). In order to achieve the correct effect for the FolderView (always $icon_size wide, whatever tall, centered vertically when in a horizontal panel, and vice versa in a vertical one), I did a)in constraintsEvent() - setMinimumSize(Global iconSize for Panel) and then setAspectRation(constrainedSquare) b)in sizeHint() - for PreferredSize, return Global iconSize for Panel. I looked it up in the Trash widget, which behaves like I've described above, and indeed, the FW icon in the panel started to behave identically. Now a)Do I need to set the size in constraintsEvent for the FormFactor change? b)Do I need to set a size in sizeHint if constraintsEvent already has a setMinimumSize? c)Why Is setMinimumSize required? Is it because the panel is trying to shrink the widget horizontally to the very minimum (if in a horizontal panel)? Remember - FW is not a PopupApplet, it is represented by a manually managed Plasma::IconWidget when in a panel. Now for some reason, the same code does not make Kickoff, the Tabbed version (which inherits popupApplet) behave identically, remaining narrow and tall in the panel. What is worse, it does not repaint on panel resize! There even is a bug somewhere complaining about that issue. Maybe there is a call that inhibits repaints somewhere? Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
constraints and size hints
Hello fellow plasma devs! Is there any docs on constraints and size hints? The documentation for the constraintsEvent method is empty (!!) and its relation to sizeHint is absolutely unclear. I've spent a day fighting kickoff trying to make it resize properly when in panel, whereas simple kickoff worjs just fine with the same code. Yesterday I spent half a day fighting those constraints and sizeHints in folderview, which does custom handling of the dropped on panel event - it can not use PopupApplet because of the Plasma class design... Now the constraints and sizeHints code which woks just fine in Trash and Folderview does not work in kickoff (which ihnerits popup applet) - and I'm afraid that if I go forward and try to fix all those "proper sizing when in the panel" bugs, I will lose a week just because I'll have to debug every case for 12 hours. I find this to be extremely unproductive. Sorry if I sound negative, but this is not what a dev should spend their time on. Doing a simple thing requires a lot of trial and error, and working without a clear understanding of the overall picture shoud be impossible for any good programmer - this is not how things should be done. Waiting for two people (mostly one) to come up on IRC to finish your code is not normal ither. Thus, I encourage those in the know to document this area with examples and a real explanation, as seen in the Qt docs. Thank you a lot in advance. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Newspaper layout problems and KNode bugs
Knode kind of sucks - no way to post, only reply, and even though I change the topic, it still posts to the same thread. Hello fellow plasma devs! I've been trying to fix (well, first reproduce) some FW bugs related to the Newspaper layout. (baseapps, runtime, workspace, libs from master or a week old). So I tried adding applets to the Newspaper layout and movng them around, and saw that the containment is absolutely broken. What's the deal? Is it no more maintained? Is it only developed in the Active branch? The problems I've seen are numerous, like applets not closing on hitting the 'X' but rather expanding in height, then closing on the second or the third click of the 'X', it was sometimes impossible to put applets in the necessary column / row, ipossible to control applets' size, applets overlapping each other, etc. I can make a screencast if you wish, but you most probably can reproduce it yourself. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Newspaper layout
Pfff sorry KNode bugs... Hello fellow plasma devs! I've been trying to fix (well, first reproduce) some FW bugs related to the Newspaper layout. (baseapps, runtime, workspace, libs from master or a week old). So I tried adding applets to the Newspaper layout and movng them around, and saw that the containment is absolutely broken. What's the deal? Is it no more maintained? Is it only developed in the Active branch? The problems I've seen are numerous, like applets not closing on hitting the 'X' but rather expanding in height, then closing on the second or the third click of the 'X', it was sometimes impossible to put applets in the necessary column / row, ipossible to control applets' size, applets overlapping each other, etc. I can make a screencast if you wish, but you most probably can reproduce it yourself. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Newspaper layout
Hello fellow plasma devs! I've been trying to fix (well, first reproduce) some FW bugs related to the Newspaper layout. (baseapps, runtime, workspace, libs from master or a week old). So I tried adding applets to the Newspaper layout and movng them around, and saw that the containment is absolutely broken. What's the deal? Is it no more maintained? Is it only developed in the Active branch? The problems I've seen are numerous, like applets not closing on hitting the 'X' but rather expanding in height, then closing on the second or the third click of the 'X', it was sometimes impossible to put applets in the necessary column / row, ipossible to control applets' size, applets overlapping each other, etc. I can make a screencast if you wish, but you most probably can reproduce it yourself. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix collapsed folderview in panels
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103915/ --- Review request for Plasma and Aaron J. Seigo. Description --- Fix folderview behavior in panels with regard to the collapsed mode. 1)Keep it small like Trash or Show desktop / Dashboard (ConstrainedSquare, minimumSize & sizeHint - code taken from Trash, which behaves properly when in a panel with regard to scaling). 2)Update icon size on panel icon size change. 3)No duplicates - if (m_iconWidget) check - the absence of such a check resulted in Really Weird(TM) things going on, like duplicates or ghost folderviews. This addresses bug 244557. http://bugs.kde.org/show_bug.cgi?id=244557 Diffs - plasma/applets/folderview/folderview.cpp c3b6e2a Diff: http://git.reviewboard.kde.org/r/103915/diff/diff Testing --- Tested the hell out of it:dragged the panel around the screen on all the 4 screen edges - no problems noticed. Changed Panel icon size in the Icons KCM, updates on the fly. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fix folderview sorting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103895/ --- (Updated Feb. 9, 2012, 6:22 p.m.) Review request for Plasma, Aaron J. Seigo, Marco Martin, Peter Penz, and Fredrik Höglund. Changes --- Fixed the comment Description --- This patch fixes the inconsistent sorting issues in FolderView. 1)It introduces explicit support for sorting by size. Prior to the change, sorting by Size was done as follows:convert the size into a string and use KStringHandler::naturalCompare(). Of course, this is not the same as a proper int comparison - FW sorted incorrectly by size. 2)Introduce one important concept:fallback to comparing the name if the main sorting column is not enough to determine a sort order. This is especially important for sorting by type - sorting by size needs this as well, but different files are way less likely to have the same size compared to the possibility of them having similar types. 3)Intoduce full three-level fallback for ensuring file name uniqueness, taken from Dolphin code. Thanks a bunch goes to Peter Penz :) 4)And of course, sort folders by the child count if sorting by size. Again, Dolphin inspired. Diffs (updated) - plasma/applets/folderview/proxymodel.cpp 4b3340e Diff: http://git.reviewboard.kde.org/r/103895/diff/diff Testing --- Tested, yields results similar to Dolphin sorting of the same folder (surpise! :) ). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
FrameSVG manual reload on themeChanged()
Hello fellow plasma developers! I'm doing a fix for folderview, related to the theme changes. In particular, I need to relayout and repaint the items using the margins in the new viewitem.svgz in the new theme. To reload the FrameSVG object manually, I need to do an update() call in the applet, which will redraw everything, including the view, and load the correct SVG. But, after that, I will recalculate the correct margins and relayout the items (as they have different sizes now), and then repaint the view completely. So we have 2 complete repaints (and one more in AppletPrivate::themeChanged(), an unconditional one.) So, I'd like to avoid this second repaint (via update()), and reload the framesvg manually. How can I do that? Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix folderview sorting
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103895/ --- Review request for Plasma, Aaron J. Seigo, Marco Martin, Peter Penz, and Fredrik Höglund. Description --- This patch fixes the inconsistent sorting issues in FolderView. 1)It introduces explicit support for sorting by size. Prior to the change, sorting by Size was done as follows:convert the size into a string and use KStringHandler::naturalCompare(). Of course, this is not the same as a proper int comparison - FW sorted incorrectly by size. 2)Introduce one important concept:fallback to comparing the name if the main sorting column is not enough to determine a sort order. This is especially important for sorting by type - sorting by size needs this as well, but different files are way less likely to have the same size compared to the possibility of them having similar types. 3)Intoduce full three-level fallback for ensuring file name uniqueness, taken from Dolphin code. Thanks a bunch goes to Peter Penz :) 4)And of course, sort folders by the child count if sorting by size. Again, Dolphin inspired. Diffs - plasma/applets/folderview/proxymodel.cpp 4b3340e Diff: http://git.reviewboard.kde.org/r/103895/diff/diff Testing --- Tested, yields results similar to Dolphin sorting of the same folder (surpise! :) ). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Take sorting order into account in ProxyModel::lessThan() in order to give folders precedence regardless of the sort order used
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103884/ --- Review request for Plasma, Aaron J. Seigo, Marco Martin, and Fredrik Höglund. Description --- Currently, after the sort order patch https://git.reviewboard.kde.org/r/103860/, folders are moved to the bottom of the sorting list with the sorting order set to "Descending". This obviously is not what the author wanted to do with the option "Folders first", so this patch tries to place folders on top of the sorting list, even if they are still sorted according to the selected sorting order (which, in my opinion, is perfectly fine). Diffs - plasma/applets/folderview/proxymodel.cpp ed28416 Diff: http://git.reviewboard.kde.org/r/103884/diff/diff Testing --- Tested, builds and works as described above. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow to choose between ascending and descending icon sort order in folderview
Marco Martin wrote: > On Mon, Feb 6, 2012 at 9:22 AM, Ignat Semenov > wrote: >> Hello fellow plasma devs! >> >> This change, while trivial, contains 2 strings, "Ascending" and >> "Desending". I've found out that the very same strings already exist in >> Dolphin, which lives in the same module as plasma-applet-folderview, kde- >> baseapps. Does that mean that this commit can be backported to 4.8? > > well, regardless of the strings, that's a feature, so i would prefer it > 4.9 only Hello Marco, In the case that I won't backport the change, will I be able to backport later commits using git cherry-pick correctly? Does Git use the diffs for cherry-picking? This is one of the reasons why I want to backport the change. Another one is, of course, that this useful feature will have a longer lifetime and more users :) Best regards, Ignst Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow to choose between ascending and descending icon sort order in folderview
Hello fellow plasma devs! This change, while trivial, contains 2 strings, "Ascending" and "Desending". I've found out that the very same strings already exist in Dolphin, which lives in the same module as plasma-applet-folderview, kde- baseapps. Does that mean that this commit can be backported to 4.8? Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Less repainting on mousePressEvent(), moseReleaseEvent() and mouseDoubleClickEvent() in FolderView::IconView
> On Feb. 5, 2012, 6:46 p.m., Fredrik Höglund wrote: > > Aside from some minor nitpicks, it looks good. I forgot to add that I couldn't optimize the right click path in mousePressEvent(). The problem is that in order to get the containment context menu, the RMB press event is propagated to the parent and triggers an unconditional repaint. I confirmed this behavior by removing all the markAreaDirty() calls in that path - then view still repainted fine. Thus, a right click repaints the whole view, voiding the optimization of that code path. Any ideas? The optimization in that branch is there just for the sake completeness - the view will repaint fully anyway. > On Feb. 5, 2012, 6:46 p.m., Fredrik Höglund wrote: > > plasma/applets/folderview/iconview.cpp, line 1803 > > <http://git.reviewboard.kde.org/r/103822/diff/1/?file=48118#file48118line1803> > > > > Make these const. There is also a whitespace error on this line. Sorry, my fault. I will make a big poster "const correctness and git diff --check" and place it on the wall above the desk ;) > On Feb. 5, 2012, 6:46 p.m., Fredrik Höglund wrote: > > plasma/applets/folderview/iconview.cpp, line 2605 > > <http://git.reviewboard.kde.org/r/103822/diff/1/?file=48118#file48118line2605> > > > > Change the name of this variable to 'rect'. Indeed, it is not necessarily dirty. Copy-paste :) - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103822/#review10360 --- On Jan. 29, 2012, 3:54 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103822/ > --- > > (Updated Jan. 29, 2012, 3:54 p.m.) > > > Review request for Plasma, Aaron J. Seigo and Marco Martin. > > > Description > --- > > This patch aims to save some repaints in FolderView::IconView on the various > mouseEvent()'s by choosing what to repaint in a bit smarter way. > > > Diffs > - > > plasma/applets/folderview/iconview.h 66ccb98 > plasma/applets/folderview/iconview.cpp 5b0cd98 > > Diff: http://git.reviewboard.kde.org/r/103822/diff/diff > > > Testing > --- > > Testing done against master, seems to behave indentically. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow to choose between ascending and descending icon sort order in folderview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103860/ --- (Updated Feb. 3, 2012, 7:16 p.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- This patch allows to select between acsending and descending icon sort order in folderview. THe QVariant() and the short-circuit if's in cg.readEntry() / cg.writeEntry() stem from the fact that Qt::SortOrder can not be converted to an int, and needs to be operated this way. Thanks go to dfaure for explaining me how to do deal with the enum problem. This addresses bug 180646. http://bugs.kde.org/show_bug.cgi?id=180646 Diffs - plasma/applets/folderview/folderview.h e458d77 plasma/applets/folderview/folderview.cpp 54ea14a Diff: http://git.reviewboard.kde.org/r/103860/diff/diff Testing --- Tested, works. Changes the icon sort direction, even though the actual sorting is sometimes broken (my next task, most probably.) Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow to choose between ascending and descending icon sort order in folderview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103860/ --- (Updated Feb. 3, 2012, 7:16 p.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Changes --- Back to strings + two static helper functions. Description --- This patch allows to select between acsending and descending icon sort order in folderview. THe QVariant() and the short-circuit if's in cg.readEntry() / cg.writeEntry() stem from the fact that Qt::SortOrder can not be converted to an int, and needs to be operated this way. Thanks go to dfaure for explaining me how to do deal with the enum problem. This addresses bug 180646. http://bugs.kde.org/show_bug.cgi?id=180646 Diffs (updated) - plasma/applets/folderview/folderview.h e458d77 plasma/applets/folderview/folderview.cpp 54ea14a Diff: http://git.reviewboard.kde.org/r/103860/diff/diff Testing --- Tested, works. Changes the icon sort direction, even though the actual sorting is sometimes broken (my next task, most probably.) Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Allow to choose between ascending and descending icon sort order in folderview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103860/ --- (Updated Feb. 3, 2012, 6:55 p.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Changes --- Strings to ints. Description --- This patch allows to select between acsending and descending icon sort order in folderview. THe QVariant() and the short-circuit if's in cg.readEntry() / cg.writeEntry() stem from the fact that Qt::SortOrder can not be converted to an int, and needs to be operated this way. Thanks go to dfaure for explaining me how to do deal with the enum problem. This addresses bug 180646. http://bugs.kde.org/show_bug.cgi?id=180646 Diffs (updated) - plasma/applets/folderview/folderview.h e458d77 plasma/applets/folderview/folderview.cpp 54ea14a Diff: http://git.reviewboard.kde.org/r/103860/diff/diff Testing --- Tested, works. Changes the icon sort direction, even though the actual sorting is sometimes broken (my next task, most probably.) Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Allow to choose between ascending and descending icon sort order in folderview
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103860/ --- Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- This patch allows to select between acsending and descending icon sort order in folderview. THe QVariant() and the short-circuit if's in cg.readEntry() / cg.writeEntry() stem from the fact that Qt::SortOrder can not be converted to an int, and needs to be operated this way. Thanks go to dfaure for explaining me how to do deal with the enum problem. This addresses bug 180646. http://bugs.kde.org/show_bug.cgi?id=180646 Diffs - plasma/applets/folderview/folderview.h e458d77 plasma/applets/folderview/folderview.cpp 54ea14a Diff: http://git.reviewboard.kde.org/r/103860/diff/diff Testing --- Tested, works. Changes the icon sort direction, even though the actual sorting is sometimes broken (my next task, most probably.) Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Apply the IconView model settings to the PopupView in the PopupView constructor for consistency
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103851/ --- Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- Apply the necessary model settings to the ProxyModel object in PopupView::PopupView(...) identically to the FolderView::init() method. This fixes bug 211002. This addresses bug 211002. http://bugs.kde.org/show_bug.cgi?id=211002 Diffs - plasma/applets/folderview/popupview.h 0243b2f plasma/applets/folderview/popupview.cpp 7fdb3f7 Diff: http://git.reviewboard.kde.org/r/103851/diff/diff Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix incorrect painting of Plasma::IconWidget on double-click with KDE set to double-click
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103831/ --- Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- Same as https://git.reviewboard.kde.org/r/103679/, but for the Plasma::IconWidget. Let the icon shrink on the double-click event, ignoring single-clicks, if KDE is set to double-click. Also take in account the corner case when the IconWidget lives in a panel and thus reacts to the clicked() signal. Diffs - plasma/widgets/iconwidget.cpp 1161cc4 Diff: http://git.reviewboard.kde.org/r/103831/diff/diff Testing --- Tested on desktop and in the panel with KDE set to both double- and single-click, works fine. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Add a link to the current desktop folder instead of an icon applet if the desktop is set to folderview on the "Add to Desktop" Kickoff action
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103830/ --- Review request for Plasma and Aaron J. Seigo. Description --- Currently, right-clicking a Kickoff/Classical application launcher entry and selectiong "Add to Desktop" puts an icon applet on the desktop. However, if the desktop is set to Folderview, it is more consistent to add a link to the currently displayed folder. This patch cheks if the "folderview" plugin is used in the desktop containment and performs a KIO::link() if that's the case. Diffs - plasma/desktop/applets/kickoff/ui/contextmenufactory.cpp 8db9655 Diff: http://git.reviewboard.kde.org/r/103830/diff/diff Testing --- Tested, works. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Less repainting on mousePressEvent(), moseReleaseEvent() and mouseDoubleClickEvent() in FolderView::IconView
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103822/ --- Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- This patch aims to save some repaints in FolderView::IconView on the various mouseEvent()'s by choosing what to repaint in a bit smarter way. Diffs - plasma/applets/folderview/iconview.h 66ccb98 plasma/applets/folderview/iconview.cpp 5b0cd98 Diff: http://git.reviewboard.kde.org/r/103822/diff/diff Testing --- Testing done against master, seems to behave indentically. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fix the issues introduced with the iconshrink patch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103685/ --- (Updated Jan. 29, 2012, 12:17 p.m.) Review request for Plasma and Aaron J. Seigo. Description --- With the iconshrink patch, I introduced an issue with the icon text clipping. The thing is, the iconview items painting code is written in a way that the text will accomodate fully only if the icon touches the top of the rect r (iconview.cpp:1050), else the text will get clipped. TO achieve this effect, Qt:AlignTop had been used. I changed that to Qt:AlignCenter and introduced the issue. This patch tries to locate the icon at the top border of the rect r (as it had been before the commit), but when the icon shrinks, it moves towards its own center, same as the first commit. ir.moveTop(r.top()); Now when the icon is shrinked, it is moved down by half the difference between its normal size and its shrinked size, which is perfectly logical. ir.translate(0, (m_drawIconShrinked && m_pressedIndex == index) ? 0.05*option.decorationSize.height() : 0); This centers the icon nicely around its own center. The text keeps "scaling" towards the top as well, as it had been before the commit. In the idle state, the text is accomodated fully, as it had been before the commit. Diffs - plasma/applets/folderview/iconview.cpp d295588 Diff: http://git.reviewboard.kde.org/r/103685/diff/diff Testing --- This is not final, there is 1 pixel offset bug in the halo drawing code. I'm going to sleep today, this review request is just to show that I'm aware that I have screwed things up and am working on getting the proper patch done. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Account for the SVG margins in FolderView::IconView::itemSize()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103735/ --- (Updated Jan. 20, 2012, 9:36 a.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- Currently, the FolderView::IconView::itemSize() method accounts for the margins only if size.width()>ts.width(), that is, the icon+margins is wider than the text. However, when ts.width()>size.width(), the margins are not accounted for, and size.width()=ts.width(). This can be observed easily. The patch adds those margins to the second operand of the comparison macro. Diffs (updated) - plasma/applets/folderview/iconview.cpp 09b9c80 Diff: http://git.reviewboard.kde.org/r/103735/diff/diff Testing --- Tested by manually changing the paintItem() code to use the itemSize() rect and observing the improved behaviour. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Account for the SVG margins in FolderView::IconView::itemSize()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103735/ --- (Updated Jan. 20, 2012, 9:35 a.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Changes --- Remove qRound() and fix the issues pointed out by aseigo. Description --- Currently, the FolderView::IconView::itemSize() method accounts for the margins only if size.width()>ts.width(), that is, the icon+margins is wider than the text. However, when ts.width()>size.width(), the margins are not accounted for, and size.width()=ts.width(). This can be observed easily. The patch adds those margins to the second operand of the comparison macro. Diffs (updated) - plasma/applets/folderview/iconview.cpp 09b9c80 Diff: http://git.reviewboard.kde.org/r/103735/diff/diff Testing --- Tested by manually changing the paintItem() code to use the itemSize() rect and observing the improved behaviour. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Account for the SVG margins in FolderView::IconView::itemSize()
> On Jan. 19, 2012, 5:24 p.m., Aaron J. Seigo wrote: > > small comment below, but looks ok otherwise. I think qRound() should go away, as per diff r2, because the code for the size calc based on the icon size a few lines above does not do this. If we qRound() the sum of the margins, we could end up getting a pixel more than the first version. What do you think? > On Jan. 19, 2012, 5:24 p.m., Aaron J. Seigo wrote: > > plasma/applets/folderview/iconview.cpp, lines 1011-1012 > > <http://git.reviewboard.kde.org/r/103735/diff/3/?file=47478#file47478line1011> > > > > hm can be cons. also, spaces around the + signs, please Will do. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103735/#review9953 --- On Jan. 19, 2012, 2:38 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103735/ > --- > > (Updated Jan. 19, 2012, 2:38 p.m.) > > > Review request for Plasma, Aaron J. Seigo and Marco Martin. > > > Description > --- > > Currently, the FolderView::IconView::itemSize() method accounts for the > margins only if size.width()>ts.width(), that is, the icon+margins is wider > than the text. However, when ts.width()>size.width(), the margins are not > accounted for, and size.width()=ts.width(). This can be observed easily. The > patch adds those margins to the second operand of the comparison macro. > > > Diffs > - > > plasma/applets/folderview/iconview.cpp 09b9c80 > > Diff: http://git.reviewboard.kde.org/r/103735/diff/diff > > > Testing > --- > > Tested by manually changing the paintItem() code to use the itemSize() rect > and observing the improved behaviour. > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Account for the SVG margins in FolderView::IconView::itemSize()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103735/ --- (Updated Jan. 19, 2012, 2:38 p.m.) Review request for Plasma, Aaron J. Seigo and Marco Martin. Changes --- Forgot a qRound(). Description --- Currently, the FolderView::IconView::itemSize() method accounts for the margins only if size.width()>ts.width(), that is, the icon+margins is wider than the text. However, when ts.width()>size.width(), the margins are not accounted for, and size.width()=ts.width(). This can be observed easily. The patch adds those margins to the second operand of the comparison macro. Diffs (updated) - plasma/applets/folderview/iconview.cpp 09b9c80 Diff: http://git.reviewboard.kde.org/r/103735/diff/diff Testing --- Tested by manually changing the paintItem() code to use the itemSize() rect and observing the improved behaviour. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Account for the SVG margins in FolderView::IconView::itemSize()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103735/ --- Review request for Plasma, Aaron J. Seigo and Marco Martin. Description --- Currently, the FolderView::IconView::itemSize() method accounts for the margins only if size.width()>ts.width(), that is, the icon+margins is wider than the text. However, when ts.width()>size.width(), the margins are not accounted for, and size.width()=ts.width(). This can be observed easily. The patch adds those margins to the second operand of the comparison macro. Diffs - plasma/applets/folderview/iconview.cpp 09b9c80 Diff: http://git.reviewboard.kde.org/r/103735/diff/diff Testing --- Tested by manually changing the paintItem() code to use the itemSize() rect and observing the improved behaviour. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fix the double-click icon shrink issue in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103679/ --- (Updated Jan. 13, 2012, 2:11 p.m.) Review request for Plasma and Aaron J. Seigo. Changes --- Fix the sticking icon bug which had slipped unnoticed into the first patch and appeared on rare occasions. Description --- With KDE set to double-click, the icons in the FolderView applet would shrink twice on double-licking them. This is a visual glitch. The patch addresses that, shrinking the icon on the mouseDoubleClicked() event instead of the mousePressed() event if KDE is set to duble-click. Diffs (updated) - plasma/applets/folderview/iconview.cpp d295588 Diff: http://git.reviewboard.kde.org/r/103679/diff/diff Testing --- Tested, works both with KDe set ot single- and double-click. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix the issues introduced with the iconshrink patch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103685/ --- Review request for Plasma and Aaron J. Seigo. Description --- With the iconshrink patch, I introduced an issue with the icon text clipping. The thing is, the iconview items painting code is written in a way that the text will accomodate fully only if the icon touches the top of the rect r (iconview.cpp:1050), else the text will get clipped. TO achieve this effect, Qt:AlignTop had been used. I changed that to Qt:AlignCenter and introduced the issue. This patch tries to locate the icon at the top border of the rect r (as it had been before the commit), but when the icon shrinks, it moves towards its own center, same as the first commit. ir.moveTop(r.top()); Now when the icon is shrinked, it is moved down by half the difference between its normal size and its shrinked size, which is perfectly logical. ir.translate(0, (m_drawIconShrinked && m_pressedIndex == index) ? 0.05*option.decorationSize.height() : 0); This centers the icon nicely around its own center. The text keeps "scaling" towards the top as well, as it had been before the commit. In the idle state, the text is accomodated fully, as it had been before the commit. Diffs - plasma/applets/folderview/iconview.cpp d295588 Diff: http://git.reviewboard.kde.org/r/103685/diff/diff Testing --- This is not final, there is 1 pixel offset bug in the halo drawing code. I'm going to sleep today, this review request is just to show that I'm aware that I have screwed things up and am working on getting the proper patch done. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Fix the double-click icon shrink issue in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103679/ --- Review request for Plasma and Aaron J. Seigo. Description --- With KDE set to double-click, the icons in the FolderView applet would shrink twice on double-licking them. This is a visual glitch. The patch addresses that, shrinking the icon on the mouseDoubleClicked() event instead of the mousePressed() event if KDE is set to duble-click. Diffs - plasma/applets/folderview/iconview.h 38cad54 plasma/applets/folderview/iconview.cpp 842e158 Diff: http://git.reviewboard.kde.org/r/103679/diff/diff Testing --- Tested, works both with KDe set ot single- and double-click. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/#review9511 --- Sorry, was a misunderstanding among me and some plasma devs. Peter, we'd like to know the rationale for exposing such an option in the configuration UI. - Ignat Semenov On Jan. 3, 2012, 5:21 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103618/ > --- > > (Updated Jan. 3, 2012, 5:21 p.m.) > > > Review request for Plasma, Aaron J. Seigo and Peter Penz. > > > Description > --- > > This patch implements a proposal from the Netrunner project > (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, > as part of my work for the project. > > This feature allows the user to hide the "+" selection marker in the > FolderView applet. It is absolutely symmetrical to the "Click to view folder" > feature. The code is similar as well. One thing I had to change is the button > hiding logic in ActionOverlay. The selection marker is positioned above the > folder peek button, so it needs a QGraphicsGridLayout::removeItem() / > ::addItem() instead of a QWidget::hide(), performed every time on the > entered() event. As the ::removeItem() has to be called only once, I had to > implement a public function that would toggle the button hiding / unhiding. > > Then, to be symmetrical, I changed the folder peek button hiding code to use > the same functions, with the same public interface in the ActionAoverlay > class. > > I hope the function addition is OK and does not violate neither incapsultion > nor the KDE class design principles. > > > Diffs > - > > plasma/applets/folderview/actionoverlay.h 056c83b > plasma/applets/folderview/actionoverlay.cpp 430e6dc > plasma/applets/folderview/folderview.h c8869b4 > plasma/applets/folderview/folderview.cpp d620a7d > plasma/applets/folderview/iconview.h 677aa76 > plasma/applets/folderview/iconview.cpp abf775e > > Diff: http://git.reviewboard.kde.org/r/103618/diff/diff > > > Testing > --- > > Works fine. > > The only problem is that following a recent Aaron's commit regarding > configChanged() / configAccepeted() in the FolderView applet circa 2 weeks > ago, the settings in this applet are applied only on Plasma restart and not > on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look > into this. This happens with any settings, not onyl the ones I added. (My > settings code is absolutely identical to the "Click to view folder" settigns > code). > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/ --- (Updated Jan. 3, 2012, 5:21 p.m.) Review request for Plasma, Aaron J. Seigo and Peter Penz. Changes --- Added Peter Penz to the discussion. Peter, we have chosen to use the global single-click settings for enabling / disabling the selection marker in the FolderView applet, contrary to exposing a configuration UI, like it's currently done in Dolphin. We Plasma devs invite you to consider doing a similar change in Dolphin for the sake of consistency. Description --- This patch implements a proposal from the Netrunner project (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, as part of my work for the project. This feature allows the user to hide the "+" selection marker in the FolderView applet. It is absolutely symmetrical to the "Click to view folder" feature. The code is similar as well. One thing I had to change is the button hiding logic in ActionOverlay. The selection marker is positioned above the folder peek button, so it needs a QGraphicsGridLayout::removeItem() / ::addItem() instead of a QWidget::hide(), performed every time on the entered() event. As the ::removeItem() has to be called only once, I had to implement a public function that would toggle the button hiding / unhiding. Then, to be symmetrical, I changed the folder peek button hiding code to use the same functions, with the same public interface in the ActionAoverlay class. I hope the function addition is OK and does not violate neither incapsultion nor the KDE class design principles. Diffs - plasma/applets/folderview/actionoverlay.h 056c83b plasma/applets/folderview/actionoverlay.cpp 430e6dc plasma/applets/folderview/folderview.h c8869b4 plasma/applets/folderview/folderview.cpp d620a7d plasma/applets/folderview/iconview.h 677aa76 plasma/applets/folderview/iconview.cpp abf775e Diff: http://git.reviewboard.kde.org/r/103618/diff/diff Testing --- Works fine. The only problem is that following a recent Aaron's commit regarding configChanged() / configAccepeted() in the FolderView applet circa 2 weeks ago, the settings in this applet are applied only on Plasma restart and not on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look into this. This happens with any settings, not onyl the ones I added. (My settings code is absolutely identical to the "Click to view folder" settigns code). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/ --- (Updated Jan. 3, 2012, 5:16 p.m.) Review request for Plasma and Aaron J. Seigo. Changes --- Implement automatic reading of the global single / double-click settings as per discussion with Marco Martin on IRC. Description --- This patch implements a proposal from the Netrunner project (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, as part of my work for the project. This feature allows the user to hide the "+" selection marker in the FolderView applet. It is absolutely symmetrical to the "Click to view folder" feature. The code is similar as well. One thing I had to change is the button hiding logic in ActionOverlay. The selection marker is positioned above the folder peek button, so it needs a QGraphicsGridLayout::removeItem() / ::addItem() instead of a QWidget::hide(), performed every time on the entered() event. As the ::removeItem() has to be called only once, I had to implement a public function that would toggle the button hiding / unhiding. Then, to be symmetrical, I changed the folder peek button hiding code to use the same functions, with the same public interface in the ActionAoverlay class. I hope the function addition is OK and does not violate neither incapsultion nor the KDE class design principles. Diffs (updated) - plasma/applets/folderview/actionoverlay.h 056c83b plasma/applets/folderview/actionoverlay.cpp 430e6dc plasma/applets/folderview/folderview.h c8869b4 plasma/applets/folderview/folderview.cpp d620a7d plasma/applets/folderview/iconview.h 677aa76 plasma/applets/folderview/iconview.cpp abf775e Diff: http://git.reviewboard.kde.org/r/103618/diff/diff Testing --- Works fine. The only problem is that following a recent Aaron's commit regarding configChanged() / configAccepeted() in the FolderView applet circa 2 weeks ago, the settings in this applet are applied only on Plasma restart and not on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look into this. This happens with any settings, not onyl the ones I added. (My settings code is absolutely identical to the "Click to view folder" settigns code). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
> On Jan. 3, 2012, 3:03 p.m., Marco Martin wrote: > > hiding the selection + icon should happen automatically when doubleclick is > > disabled, i don't see a valid use case to be able to configure it > > idependently > > Shaun Reich wrote: > I think/hope you meant that the selection icon should be hidden when > double is *enabled*. i.e. singleclick is on. Since the selection icon is the > most useful when you can't click to select (single-click mode only). > > That said, the option has existed since forever in Dolphin, if we're > going to bring a feature over, we'd better bring the whole thing and not just > bits and pieces to make it inconsistent (as the patcher duly noted. > > Marco Martin wrote: > yes, selection icon enabled when double click is disabled. > > but honestly, i rather see it as a problem of dolphin if it lets > configure such a thing. > only thing that would make sense (since removing that from dolphin would > probably cause a revolution as the removal of anything) is to read that > setting from the dolphin configuration itself rather than the applet own one > > Shaun Reich wrote: > Well, given how stringent Peter is on settings, I'm sure he didn't add it > without quite some thought and good reasons ;) > > But yes, for unification's sake, it definitely makes the most sense to > make it as global as can be. Otherwise they have crap to configure in dolphin > and then every folderview they own. Plus it keeps it clean; good idea. > > Ignat Semenov wrote: > OK. So, are you fine with the implementation code? > > If yes, I will commit the backend part (ActionOverlay) separately? The > GUI bits may come in later. > > Please, give me a final decision on this. Do I maybe need to wait for > Aaron to approve this (a week, that is)? OK, I will post a diff with automatic double-click detection soon. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/#review9500 --- On Jan. 3, 2012, 1:03 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103618/ > --- > > (Updated Jan. 3, 2012, 1:03 p.m.) > > > Review request for Plasma and Aaron J. Seigo. > > > Description > --- > > This patch implements a proposal from the Netrunner project > (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, > as part of my work for the project. > > This feature allows the user to hide the "+" selection marker in the > FolderView applet. It is absolutely symmetrical to the "Click to view folder" > feature. The code is similar as well. One thing I had to change is the button > hiding logic in ActionOverlay. The selection marker is positioned above the > folder peek button, so it needs a QGraphicsGridLayout::removeItem() / > ::addItem() instead of a QWidget::hide(), performed every time on the > entered() event. As the ::removeItem() has to be called only once, I had to > implement a public function that would toggle the button hiding / unhiding. > > Then, to be symmetrical, I changed the folder peek button hiding code to use > the same functions, with the same public interface in the ActionAoverlay > class. > > I hope the function addition is OK and does not violate neither incapsultion > nor the KDE class design principles. > > > Diffs > - > > plasma/applets/folderview/actionoverlay.h 056c83b > plasma/applets/folderview/actionoverlay.cpp 430e6dc > plasma/applets/folderview/folderview.h c8869b4 > plasma/applets/folderview/folderview.cpp d620a7d > plasma/applets/folderview/folderviewDisplayConfig.ui e7a5e46 > plasma/applets/folderview/iconview.h 677aa76 > plasma/applets/folderview/iconview.cpp abf775e > > Diff: http://git.reviewboard.kde.org/r/103618/diff/diff > > > Testing > --- > > Works fine. > > The only problem is that following a recent Aaron's commit regarding > configChanged() / configAccepeted() in the FolderView applet circa 2 weeks > ago, the settings in this applet are applied only on Plasma restart and not > on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look > into this. This happens with any settings, not onyl the ones I added. (My > settings code is absolutely identical to the "Click to view folder" settigns > code). > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
> On Jan. 3, 2012, 3:03 p.m., Marco Martin wrote: > > hiding the selection + icon should happen automatically when doubleclick is > > disabled, i don't see a valid use case to be able to configure it > > idependently > > Shaun Reich wrote: > I think/hope you meant that the selection icon should be hidden when > double is *enabled*. i.e. singleclick is on. Since the selection icon is the > most useful when you can't click to select (single-click mode only). > > That said, the option has existed since forever in Dolphin, if we're > going to bring a feature over, we'd better bring the whole thing and not just > bits and pieces to make it inconsistent (as the patcher duly noted. > > Marco Martin wrote: > yes, selection icon enabled when double click is disabled. > > but honestly, i rather see it as a problem of dolphin if it lets > configure such a thing. > only thing that would make sense (since removing that from dolphin would > probably cause a revolution as the removal of anything) is to read that > setting from the dolphin configuration itself rather than the applet own one > > Shaun Reich wrote: > Well, given how stringent Peter is on settings, I'm sure he didn't add it > without quite some thought and good reasons ;) > > But yes, for unification's sake, it definitely makes the most sense to > make it as global as can be. Otherwise they have crap to configure in dolphin > and then every folderview they own. Plus it keeps it clean; good idea. OK. So, are you fine with the implementation code? If yes, I will commit the backend part (ActionOverlay) separately? The GUI bits may come in later. Please, give me a final decision on this. Do I maybe need to wait for Aaron to approve this (a week, that is)? - Ignat --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/#review9500 --- On Jan. 3, 2012, 1:03 p.m., Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103618/ > --- > > (Updated Jan. 3, 2012, 1:03 p.m.) > > > Review request for Plasma and Aaron J. Seigo. > > > Description > --- > > This patch implements a proposal from the Netrunner project > (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, > as part of my work for the project. > > This feature allows the user to hide the "+" selection marker in the > FolderView applet. It is absolutely symmetrical to the "Click to view folder" > feature. The code is similar as well. One thing I had to change is the button > hiding logic in ActionOverlay. The selection marker is positioned above the > folder peek button, so it needs a QGraphicsGridLayout::removeItem() / > ::addItem() instead of a QWidget::hide(), performed every time on the > entered() event. As the ::removeItem() has to be called only once, I had to > implement a public function that would toggle the button hiding / unhiding. > > Then, to be symmetrical, I changed the folder peek button hiding code to use > the same functions, with the same public interface in the ActionAoverlay > class. > > I hope the function addition is OK and does not violate neither incapsultion > nor the KDE class design principles. > > > Diffs > - > > plasma/applets/folderview/actionoverlay.h 056c83b > plasma/applets/folderview/actionoverlay.cpp 430e6dc > plasma/applets/folderview/folderview.h c8869b4 > plasma/applets/folderview/folderview.cpp d620a7d > plasma/applets/folderview/folderviewDisplayConfig.ui e7a5e46 > plasma/applets/folderview/iconview.h 677aa76 > plasma/applets/folderview/iconview.cpp abf775e > > Diff: http://git.reviewboard.kde.org/r/103618/diff/diff > > > Testing > --- > > Works fine. > > The only problem is that following a recent Aaron's commit regarding > configChanged() / configAccepeted() in the FolderView applet circa 2 weeks > ago, the settings in this applet are applied only on Plasma restart and not > on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look > into this. This happens with any settings, not onyl the ones I added. (My > settings code is absolutely identical to the "Click to view folder" settigns > code). > > > Thanks, > > Ignat Semenov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/ --- (Updated Jan. 3, 2012, 1:03 p.m.) Review request for Plasma and Aaron J. Seigo. Changes --- Remove duplicate code. Description --- This patch implements a proposal from the Netrunner project (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, as part of my work for the project. This feature allows the user to hide the "+" selection marker in the FolderView applet. It is absolutely symmetrical to the "Click to view folder" feature. The code is similar as well. One thing I had to change is the button hiding logic in ActionOverlay. The selection marker is positioned above the folder peek button, so it needs a QGraphicsGridLayout::removeItem() / ::addItem() instead of a QWidget::hide(), performed every time on the entered() event. As the ::removeItem() has to be called only once, I had to implement a public function that would toggle the button hiding / unhiding. Then, to be symmetrical, I changed the folder peek button hiding code to use the same functions, with the same public interface in the ActionAoverlay class. I hope the function addition is OK and does not violate neither incapsultion nor the KDE class design principles. Diffs (updated) - plasma/applets/folderview/actionoverlay.h 056c83b plasma/applets/folderview/actionoverlay.cpp 430e6dc plasma/applets/folderview/folderview.h c8869b4 plasma/applets/folderview/folderview.cpp d620a7d plasma/applets/folderview/folderviewDisplayConfig.ui e7a5e46 plasma/applets/folderview/iconview.h 677aa76 plasma/applets/folderview/iconview.cpp abf775e Diff: http://git.reviewboard.kde.org/r/103618/diff/diff Testing --- Works fine. The only problem is that following a recent Aaron's commit regarding configChanged() / configAccepeted() in the FolderView applet circa 2 weeks ago, the settings in this applet are applied only on Plasma restart and not on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look into this. This happens with any settings, not onyl the ones I added. (My settings code is absolutely identical to the "Click to view folder" settigns code). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Add an option for disabling / enabling the selection marker in the FolderView applet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103618/ --- Review request for Plasma and Aaron J. Seigo. Description --- This patch implements a proposal from the Netrunner project (netrunner-os.com), a KUbuntu derivative, aimed at polishing the KDE desktop, as part of my work for the project. This feature allows the user to hide the "+" selection marker in the FolderView applet. It is absolutely symmetrical to the "Click to view folder" feature. The code is similar as well. One thing I had to change is the button hiding logic in ActionOverlay. The selection marker is positioned above the folder peek button, so it needs a QGraphicsGridLayout::removeItem() / ::addItem() instead of a QWidget::hide(), performed every time on the entered() event. As the ::removeItem() has to be called only once, I had to implement a public function that would toggle the button hiding / unhiding. Then, to be symmetrical, I changed the folder peek button hiding code to use the same functions, with the same public interface in the ActionAoverlay class. I hope the function addition is OK and does not violate neither incapsultion nor the KDE class design principles. Diffs - plasma/applets/folderview/actionoverlay.h 056c83b plasma/applets/folderview/actionoverlay.cpp 430e6dc plasma/applets/folderview/folderview.h c8869b4 plasma/applets/folderview/folderview.cpp d620a7d plasma/applets/folderview/folderviewDisplayConfig.ui e7a5e46 plasma/applets/folderview/iconview.h 677aa76 plasma/applets/folderview/iconview.cpp abf775e Diff: http://git.reviewboard.kde.org/r/103618/diff/diff Testing --- Works fine. The only problem is that following a recent Aaron's commit regarding configChanged() / configAccepeted() in the FolderView applet circa 2 weeks ago, the settings in this applet are applied only on Plasma restart and not on the fly after hitting the Appy or Ok buttons. Aaron, please, have a look into this. This happens with any settings, not onyl the ones I added. (My settings code is absolutely identical to the "Click to view folder" settigns code). Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
FolderView Overlay Incapsulation Question
Hello Plasma devs! I wanted to ask this question on IRC, but nobody's there at the moment. So I'm asking here instead. I'm implementing the option to hide the selection marker in FolderView as per the proposal from the NetRunner project. (netrunner-os.com). (I'm now working for that project, doing various KDE polish tasks. This is my first task. I will soon publish a review request for it.) I have located all the relevant code and implemented all the necessary bits but one. The Overlay buttons are in a QGraphicsGridLayout. The "Click to view" button (the lower one) uses the QWidget::hide() method in hoverEvent() and successfully hides itself. The Selection button, on the other hand, is located above the first button, and QWidget::hide() results in an **ugly gap**. Thus I have to use the QGraphicsGridLayout::removeItem() and ::addItem() instead of QWidget::hide(). Now these functions have to be called once on settings change, not every time on hoverEvent(). This means I have to implement a public method in ActionOverlay void ActionOverlay::setShowSelectionMarker(bool show); which would add / remove the button from the layout and hide it, resulting in a proper button alignment. Now if I implement this method, for consistency and symmetry, I will have to do the same for the ClickToView button: void ActionOverlay::setShowClickToViewButton(bool show); There are currently no such methods. It's done differently. Does this mean that my method is incorrect? Will my implementation break incapsulation or the existing class design? Please approve or disapprove of this proposal, as I have to finish coding this (dead simple) task soon and go forward. The other problem with FW is that following a recent commit from Aaron Seigo, the configAccepted() method no longer aplies settings, which is perfectly fine per se. But, now when we click Apply or OK, the settings are written to the disk, but are **not** applied. Tested in trunk. This needs some solution as well. Best regards, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Apply the new settings in Folderview on clicking the "Apply" button
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103545/ --- Review request for Plasma and Aaron J. Seigo. Description --- Currently, after Aaron's patch https://projects.kde.org/projects/kde/kde-baseapps/repository/revisions/10a7c8aaa06d73a1415d128f25000f7335a59d25, certain FolderView settings are not changed on the fly (but instead change only after a plasma-desktop restart) after I hit the "Apply" button. E.g. the "Click to enter folder", or "Show Desktop/Home/cutom location", and others. I think this is because the settings are written to disk, but are not applied in configAccepted(). This patch adds a configChanged() call right before the configAccepted() function end. Diffs - plasma/applets/folderview/folderview.cpp d620a7d Diff: http://git.reviewboard.kde.org/r/103545/diff/diff Testing --- Works fine after the patch. Settings are indeed changed after hitting the "Apply" button. I'm not sure, however, whether the configChanged() should go before the timer or after it. My guess is that the timer call is there in order to allow the newly written settings to sync to the hard disk, so I put the call at the very end of the accepted() method. Thanks, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Fïx two little bugs: one with default contrast settings and one with plasma popup alignment.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103529/#review9282 --- Ship it! Given that two developers have both given positive feedback on the relevant parts of the patch, although independently, I can use the right to give the review a ship it! Nikita Churaev, do you have a developer account to commit the changes? If not, you can request a developer account at your identity.kde.org personal page using a similarly named button. - Ignat Semenov On Dec. 24, 2011, 5:35 p.m., Nikita Churaev wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103529/ > --- > > (Updated Dec. 24, 2011, 5:35 p.m.) > > > Review request for kdelibs and Plasma. > > > Description > --- > > Contrast bug: KGlobalSettings::contrast() returns different default value > than KGlobalSettings::contrastF(). Oxygen uses contrastF() while KDE color > control module uses contrast(), so when the user first uses color settings, > contrast appears to change. > > Plasma popup bug: Right-aligned popups are one pixel away from right edge of > the screen and top-aligned popups (when panel is on top) are one pixel inside > panel. This is because the bug in QRect, where right() and bottom() value > return value that is less than the true value by one. This is a feature-bug > that Qt developers aren't going to fix because of compatiability reasons. My > patch just applies + 1 compensation to all expressions that use > QRect::right() and QRect::bottom(). > > > Diffs > - > > kdeui/kernel/kglobalsettings.cpp 819b314 > plasma/corona.cpp 366a9df > > Diff: http://git.reviewboard.kde.org/r/103529/diff/diff > > > Testing > --- > > > Thanks, > > Nikita Churaev > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: KWin GLES needs testers
Hi Martin, Thank you and happy New Year to you as well :) Today I built your branch. My system is: Fedora 14 KDE trunk 8800 GTS 320 Mb NVIDIA 260.19 (latest) First of all, don't know what it built against - hope it's GL 2.0 because it's what the driver seems to support. It works! But there's a few bugs: the thumbnails are broken (as in when there's e.g. Konsole with build output), those lines are riddled with holes and not "crisp" at all (btw, nice string in the config dialog :)) kwin crashes when I change texture scaling method and something else in one of the above comboboxes and hit apply - reliably. Everything (KDEbase and KDElibs) are from trunk, today's trunk, actually. blur works jaggies in coverswitch (antialiasing? is that possible at all? it's been there actually since 4.0 I guess :)) everything moves pretty smooth, haven't seen any jerkies yet That's it after half an hour of normal use, no crashes except for the mentioned crash in the config dialog. Regards, Ignat Semenov P.S. As you say, nouveau allows for GLES (didn't know about that, btw.) Can test this as well if necessary. (Not sure about the prerequisites, that could take some time.) ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: The state of KWin
Well then I'm very glad to know that there are chances :) Anyway, you should have some projects planned, like great code transitions similar to those in KWin (see the original post) or something else, which you _definitely_ want to do and look for the manpower to accomplish those. It's like the variant with the highest probability to get into GSOC, I think :) Anyway, I _may_ be coming back for a very small period of time during winter holidays, but not sure yet. But that's OT anyway. Going to do mostly UI papercuts hunting, though. (The kind of contribution that takes the most time to investigate and the least amount of lines to submit after solving the problem, don't you think.) Cheers, Ignat Semenov ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: The state of KWin
Hello guys and plasma-devel! You might still remember me from the summer days, a guy most of the time hanging out in #plasma and #oxygen as thorgt. ATM I've got a job, essentially an internship, here in Russia which most likely is going to continue at lest till summer, when... well when I'm finally free. So Martin is talking about known KDE developers or near-developers (at least I hope they're included as well :)), especially students. I'd like to say you already have one, looking to find a GSOC application and maybe (in the long-term) something in the area of full-time or part-time KDE developer, sorta what you call a sponsorship or something, hope that's not too dire, but that's what I'm aiming for. So here's hoping to get involved on a regular and official basis from a Russian tech guy, C++ fan and Qt/KDE fanboy as well, although not an expert nor a pro yet, and definitely a Linux fan, as well as a long time Fedora user :) Just so you could possibly look forward to one more unit of manpower in summer. Is it possible to get into GSOC reliably in this or other way? Who decides that anyway, you or some GSOC committee? You might say that it's too early, but it's better to do things earlier that later anyway, don't you think? And mind you I'm from Russia, but that should not be a problem anyway, hope so at least. Cheers, Ignat Semenov aka thorGT ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Embed "Virtual Desktops" KCM into the pager configuration dialog
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4154/ --- Review request for Plasma and Aaron Seigo. Summary --- This patch makes use of KCModuleProxy class and embeds the virtual desktops KCM directly into the pager configuration dialog, removing the "Configure desktops" button. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/CMakeLists.txt 1130818 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.h 1130818 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1130818 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pagerConfig.ui 1130818 Diff: http://reviewboard.kde.org/r/4154/diff Testing --- There is a strange problem. Wheh I build it on my machine, if I press OK in the embedded KCM after changging something in it, Plasma quits. No crash, just quits. Marco Martin has applied the patch and he has no such problem. Please investigate. Until solved this request is a draft. Thanks, Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 1 : The Pager
On 5/25/10, Aaron Seigo wrote: > > "so it's not crammed to the left side as before" > > which is how all the other dialogs are. please don't change one dialog to > some random new style without first discussing it with us on > plasma-de...@kde.org. thanks. if we change this one, then they all need to > be changed. I am perfectly aware of the fact that _every_ dialog has to be redone in case we decide on changing this one. Maybe I've been unclear about that, sorry. This is just an example. > > "in Plasma, e.g. in Task Manager" > > the Task Manager settings dialog is simply incorrect. look through the other > widgets and you'll see. please coordinate with myself and Davide Bettio who > has done a lot of this work in the recent past. > I suggest to discuss this with Davide Bettio when he is availiable again. > "how to improve what we have in Plasma in this area" > > please bring your conclusions to plasma-d...@kde.org before spending more > time implementing them. it will save everyone time and effort. > > > - Aaron Well, this one should be more of a mockup, yes. I'd like to know what other graphics design people think about this. If decided upon, then the conversion should be easy and can be done in less than a day, so it's not that difficult, IMO. Regards Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 2: The Default Button
> On 2010-05-25 17:44:11, Aaron Seigo wrote: > > i'm fine with the change to the default button, but the separator should > > remain consistent with the defaults of the KDE Dev Platform. so this is a > > "half ship it" ;) please commit the change to the default button, but not > > the separator change. > > Ignat Semenov wrote: > Please, show me a config dialog with a separator in KDE. No offense, but > I really haven't found one. Again, it creates visual noise and again, we > agreed on removing it. And I have no SVN account atm, should I apply for one? - Ignat --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4130/#review5860 ------- On 2010-05-24 18:57:54, Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/4130/ > --- > > (Updated 2010-05-24 18:57:54) > > > Review request for Plasma and Marco Martin. > > > Summary > --- > > This is further prettification of the Plasmoid Configuration Dialogs. As > discussed on IRC, the "Default" button is removed until we port plasmoid > config to KConfigXT and the separator is removed as well. > > > Diffs > - > > /trunk/KDE/kdelibs/plasma/applet.cpp 1130151 > > Diff: http://reviewboard.kde.org/r/4130/diff > > > Testing > --- > > Builds fine. The dialog has no "Default" button and no separator at the > bottom. > > > Thanks, > > Ignat > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 2: The Default Button
> On 2010-05-25 17:44:11, Aaron Seigo wrote: > > i'm fine with the change to the default button, but the separator should > > remain consistent with the defaults of the KDE Dev Platform. so this is a > > "half ship it" ;) please commit the change to the default button, but not > > the separator change. Please, show me a config dialog with a separator in KDE. No offense, but I really haven't found one. Again, it creates visual noise and again, we agreed on removing it. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4130/#review5860 ----------- On 2010-05-24 18:57:54, Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/4130/ > --- > > (Updated 2010-05-24 18:57:54) > > > Review request for Plasma and Marco Martin. > > > Summary > --- > > This is further prettification of the Plasmoid Configuration Dialogs. As > discussed on IRC, the "Default" button is removed until we port plasmoid > config to KConfigXT and the separator is removed as well. > > > Diffs > - > > /trunk/KDE/kdelibs/plasma/applet.cpp 1130151 > > Diff: http://reviewboard.kde.org/r/4130/diff > > > Testing > --- > > Builds fine. The dialog has no "Default" button and no separator at the > bottom. > > > Thanks, > > Ignat > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Properly hide KConfigDialog page headers when there is no header text set
> On 2010-05-25 17:41:37, Aaron Seigo wrote:
> > having the header there is intentional and desired. the point is to show
> > the name of the page there if a replacement string isn't explicitly
> > defined. i'm not sure what problem is trying to be solved here, exactly,
> > but this changes the behaviour of this dialog everywhere and isn't
> > acceptable as such. perhaps we can discuss on plasma-devel@ what the actual
> > goal here is with this patch and try to come up with a workable solution.
There was a comment somewhere in KPageWidgetItem docs that the behaviour I've
restored is the correct one. See APIdocs, kde45.ach for now, search for
KPageWidgetItem.
void KPageWidgetItem::setHeader ( const QString & header )
Sets the header of the page widget item.
If setHeader(QString()) is used, what is the default if the header does not got
set explicit, then the defined name() will also be used for the header. If
setHeader("") is used, the header will be hidden even if the
KPageView::FaceType is something else then Tabbed.
Parameters:
header Header of the page widget item.
Again, discussed with pinheiro and notmart on #oxygen. We agreed to avoid bad
information duplication. I'd really likt to see it that way.
- Ignat
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4124/#review5859
-------
On 2010-05-25 18:07:48, Ignat Semenov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4124/
> ---
>
> (Updated 2010-05-25 18:07:48)
>
>
> Review request for kdelibs and Plasma.
>
>
> Summary
> ---
>
> This is essentially a workaround for the broken logic in
> KPageWidgetItem::setHeader(). When no header text was supplied in
> KConfigDialog::addPage(), a KPageWidgetItem was created with QString() as the
> header text, which caused the header to be set, as setHeader checks for
> header.isNull() and thus requires a QString(""). This patch makes sure that
> when header==QString() in addPage, setHeader( QString("") ) is called.
>
>
> Diffs
> -
>
> /trunk/KDE/kdelibs/kdeui/dialogs/kconfigdialog.cpp 1130107
>
> Diff: http://reviewboard.kde.org/r/4124/diff
>
>
> Testing
> ---
>
> After applying the patch, no headers appear in the Plasma configuration
> dialogs, which use KConfigDialog::addPage(QWidget *, QString &, QString &).
> It doesn't break BC as the cnahge is done in a private class.
>
>
> Thanks,
>
> Ignat
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Properly hide KConfigDialog page headers when there is no header text set
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4124/
---
(Updated 2010-05-25 18:07:48.304935)
Review request for kdelibs and Plasma.
Summary
---
This is essentially a workaround for the broken logic in
KPageWidgetItem::setHeader(). When no header text was supplied in
KConfigDialog::addPage(), a KPageWidgetItem was created with QString() as the
header text, which caused the header to be set, as setHeader checks for
header.isNull() and thus requires a QString(""). This patch makes sure that
when header==QString() in addPage, setHeader( QString("") ) is called.
Diffs
-
/trunk/KDE/kdelibs/kdeui/dialogs/kconfigdialog.cpp 1130107
Diff: http://reviewboard.kde.org/r/4124/diff
Testing
---
After applying the patch, no headers appear in the Plasma configuration
dialogs, which use KConfigDialog::addPage(QWidget *, QString &, QString &).
It doesn't break BC as the cnahge is done in a private class.
Thanks,
Ignat
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 1 : The Pager
> On 2010-05-25 17:28:45, Aaron Seigo wrote: > > what is this patch fixing in the current pager? > > > > the checkbox is precisely how we do them in plasma, and the reasons for > > this is a combination of visual scanability and aesthetics. also, since > > we're in string freeze, we can't change strings and "Display icons:" with a > > ':' looks (and is) incorrect when placed to the right of the checkbox. > > > > why is the configure desktops button so large? > > > > honestly, if i were to suggest an improvement it would be this: get rid of > > the Configure Desktops button and simply include the desktops kcm in the > > dialog directly as a new page, just as we do elsewhere for, e.g., the > > automount and device action kcms in the device notifier plasmoid. OK, will embed the KCM, today or tomorrow, depends on time. What's it fixing? A bit of aesthetical issues. It has a layout now, so it's not crammed to the left side as before. It's a proper QFormLayout as intended (this is the preferrable way to do that AFAIK). You can resize it freely -> consistency, kinda. The button is centered now, but well, you'd like to see it removed and I agree. The checkbox is done this way in most other KDE config dialogs and in Plasma, e.g. in Task Manager. I think it's more consistent and balanced that way. The last argument (not sure if correct at all but still) is that it's recommended to do it that way in the coolest user interface document in the world - the Apple HIG. Well, yes, I know, we're not copycats, but pinheiro approved it (we discussed it on the IRC in #oxygen channel, got logs just in case ;) don't get me wrong, was a long discussion on how to improve what we have in Plasma in this area, it raised quite some questions). String freeze - well, let's do it on the left for 4.5 and then on the right. I'd really like to see it that way. - Ignat --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4119/#review5858 --- On 2010-05-23 22:52:04, Ignat Semenov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/4119/ > --- > > (Updated 2010-05-23 22:52:04) > > > Review request for Plasma. > > > Summary > --- > > This is the final version of the Pager configuration Dialog patch. Changes: > > - Brought back radiobuttons > - Changed the "Display icons" checkbox to respect KDE style > - Introduced a ton of layouts and set a top-level layout so that the dialog > scales properly now > - Centered the "Configure Desktops" button and made it huge > > I plan to fix the layout issues for all plasmoid configuration dialogs, as > time allows. > Now I'd like to know what's the rationale behind headers in configuration > dialogs. They're redundant from my point of view as they duplicate the > information which is on the left in the list of configuration pages. Maybe > they can be removed? Not it Plasma, of course, but in KConfigurationDialog > source. > > > Diffs > - > > /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1129710 > /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pagerConfig.ui > 1129710 > > Diff: http://reviewboard.kde.org/r/4119/diff > > > Testing > --- > > Built it and it really scales properly now. There is only a minor problem > with Oxygen style and I've already filed a bug against Oxygen. > > > Screenshots > --- > > New configuration dialog > http://reviewboard.kde.org/r/4119/s/410/ > > > Thanks, > > Ignat > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Change device notifier tooltips to respect plasma tooltip style
And maybe no text at all for the separators? Like one more font face and not aligned with anything either. (Yes, I like to remove stuff ;) On Tue, May 25, 2010 at 8:22 PM, Aaron J. Seigo wrote: > On May 22, 2010, Jacopo De Simoi wrote: >> I'm playing around with that now, and here's what I got so far... >> how do you like it? > > it puts a third size of icon in the dialog, and it isn't really aligned with > anything visually. > > my recommendation would be to just ditch the icon altogether. it doesn't > really provide any content. center the text at the top. only show the > separators between devices if there is more than one type. > > 0.02 :) > > -- > Aaron J. Seigo > humru othro a kohnu se > GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 > > KDE core developer sponsored by Qt Development Frameworks > > ___ > Plasma-devel mailing list > Plasma-devel@kde.org > https://mail.kde.org/mailman/listinfo/plasma-devel > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Plasmoid Configuration Dialogs Saga Part 2: The Default Button
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4130/ --- Review request for Plasma and Marco Martin. Summary --- This is further prettification of the Plasmoid Configuration Dialogs. As discussed on IRC, the "Default" button is removed until we port plasmoid config to KConfigXT and the separator is removed as well. Diffs - /trunk/KDE/kdelibs/plasma/applet.cpp 1130151 Diff: http://reviewboard.kde.org/r/4130/diff Testing --- Builds fine. The dialog has no "Default" button and no separator at the bottom. Thanks, Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Properly hide KConfigDialog page headers when there is no header text set
---
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4124/
---
(Updated 2010-05-24 16:55:36.267182)
Review request for kdelibs and Plasma.
Summary
---
This is essentially a workaround for the broken logic in
KPageWidgetItem::setHeader(). When no header text was supplied in
KConfigDialog::addPage(), a KPageWidgetItem was created with QString() as the
header text, which caused the header to be set, as setHeader checks for
header.isNull() and thus requires a QString(""). This patch makes sure that
when header==QString() in addPage, setHeader( QString("") ) is called.
Diffs
-
/trunk/KDE/kdelibs/kdeui/dialogs/kconfigdialog.cpp 1130107
Diff: http://reviewboard.kde.org/r/4124/diff
Testing
---
After applying the patch, no headers appear in the Plasma configuration
dialogs, which use KConfigDialog::addPage(QWidget *, QString &, QString &).
It doesn't break BC as the cnahge is done in a private class.
Thanks,
Ignat
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 1 : The Pager
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4119/ --- (Updated 2010-05-23 22:52:04.976831) Review request for Plasma. Summary --- This is the final version of the Pager configuration Dialog patch. Changes: - Brought back radiobuttons - Changed the "Display icons" checkbox to respect KDE style - Introduced a ton of layouts and set a top-level layout so that the dialog scales properly now - Centered the "Configure Desktops" button and made it huge I plan to fix the layout issues for all plasmoid configuration dialogs, as time allows. Now I'd like to know what's the rationale behind headers in configuration dialogs. They're redundant from my point of view as they duplicate the information which is on the left in the list of configuration pages. Maybe they can be removed? Not it Plasma, of course, but in KConfigurationDialog source. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1129710 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pagerConfig.ui 1129710 Diff: http://reviewboard.kde.org/r/4119/diff Testing --- Built it and it really scales properly now. There is only a minor problem with Oxygen style and I've already filed a bug against Oxygen. Screenshots --- New configuration dialog http://reviewboard.kde.org/r/4119/s/410/ Thanks, Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Plasmoid Configuration Dialogs Saga Part 1 : The Pager
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4119/ --- (Updated 2010-05-23 18:32:54.867447) Review request for Plasma. Changes --- Added QGroupBox to make the radiobuttons exclusive. They somehow got deleted in process. Summary --- This is the final version of the Pager configuration Dialog patch. Changes: - Brought back radiobuttons - Changed the "Display icons" checkbox to respect KDE style - Introduced a ton of layouts and set a top-level layout so that the dialog scales properly now - Centered the "Configure Desktops" button and made it huge I plan to fix the layout issues for all plasmoid configuration dialogs, as time allows. Now I'd like to know what's the rationale behind headers in configuration dialogs. They're redundant from my point of view as they duplicate the information which is on the left in the list of configuration pages. Maybe they can be removed? Not it Plasma, of course, but in KConfigurationDialog source. Diffs (updated) - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1129710 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pagerConfig.ui 1129710 Diff: http://reviewboard.kde.org/r/4119/diff Testing --- Built it and it really scales properly now. There is only a minor problem with Oxygen style and I've already filed a bug against Oxygen. Thanks, Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Plasmoid Configuration Dialogs Saga Part 1 : The Pager
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/4119/ --- Review request for Plasma. Summary --- This is the final version of the Pager configuration Dialog patch. Changes: - Brought back radiobuttons - Changed the "Display icons" checkbox to respect KDE style - Introduced a ton of layouts and set a top-level layout so that the dialog scales properly now - Centered the "Configure Desktops" button and made it huge I plan to fix the layout issues for all plasmoid configuration dialogs, as time allows. Now I'd like to know what's the rationale behind headers in configuration dialogs. They're redundant from my point of view as they duplicate the information which is on the left in the list of configuration pages. Maybe they can be removed? Not it Plasma, of course, but in KConfigurationDialog source. Diffs - /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pager.cpp 1129710 /trunk/KDE/kdebase/workspace/plasma/desktop/applets/pager/pagerConfig.ui 1129710 Diff: http://reviewboard.kde.org/r/4119/diff Testing --- Built it and it really scales properly now. There is only a minor problem with Oxygen style and I've already filed a bug against Oxygen. Thanks, Ignat ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Pager Configuration Dialog
Patch version 2.Regards,Ignat new_non_expanding.patch Description: Binary data ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Pager Configuration Dialog
Nevermind, I've cheated and it works now :)Was as simple aswidget->setLayout(ui.verticalLayout);in createConfigurationInterface().So here's the patch version 1. plasma-devel says "no >4o KB messages", so second patch comes in the second letter.Please tell me what you like more and what fits better into KDE style.Going on to task manager.Regards,Ignat new.patch Description: Binary data ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Pager Configuration Dialog
Hi all,I'm currently trying to improve the Pager Configuration Dialog. Actually, it's not the only one that should be improved IMHO, but well, that's a different story, now I'm fighting the Pager plasmoid. So far so good. I've decided to do this :- Change radiobuttons to comboboxes, as any other config dialog in KDE uses that if there are three o more options available, and I remember some article about that on the Planet.- Lay the buttons out nicely- Use a QFormLayout (do it right TM)- Center the button, one more layout- Use a top-level layout : as the dialog is resizable, if the user resize it, it's not going to look so nice, so a layout is needed.But there is a problem. It uses KConfigDialog, and passes a widget that is created from the .ui file. It appears that setupUi() doesn't honor any top-level layouts, so to create one, you must do it by hand! In code, I mean. Finally, I get this when I try to build the configuration dialog with the new .ui file:http://imagebin.ca/view/Fh1QOPS.htmlThe .ui file is attached, as well as a patch for the source files (radiobuttons -> comboboxes) and the .ui file (in case you prefer a patch).Now the question is this:1) What's that bad clipping2) What should I do about the top-level layout (kinda the same question)In the meantime I'm going to fight the other ones.Looking forward to your answerRegards,Ignat pagerConfig.ui Description: Binary data new.patch Description: Binary data ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
