Re: Request for review

[Martin Klapetek]

On Mon, Feb 17, 2014 at 8:01 AM, Conny Marco Menebröcker <
ad...@menebroecker-web.de> wrote:

> I will read the tutorial and decide afterwards, if I will change something.
> I don't know much about QML, I thought it was only a specific programming
> language for QT development.
> So, currently I don't understand the advantage. But we will see.
>

The advantages are:
 * way simpler code than with QGV
 * it also works way better than QGV (which had lots and lots of quirks)
 * no need to actually compile your code -> faster testing and deployment
 * code is easier to maintain
 * the power of declarative language -> you don't have to be a programmer
to make good UIs

It's not a language for Qt's development, it's a general programming
language anyone can use for anything, based on JavaScript :)

Cheers
-- 
Martin Klapetek | KDE Developer
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Request for review

[Conny Marco Menebröcker]

Hello Martin,



Am 16.02.2014 17:41, schrieb Martin Klapetek:

Hey,

-- Forwarded message --
From: Conny Marco Menebröcker mailto:ad...@menebroecker-web.de>>
Date: Sat, Feb 15, 2014 at 7:44 PM
Subject: Request for review
To: kde-de...@kde.org 


Hello all,


I have written my first plasmoid. I called it PlasmaTaskViewer,
because it views the todos of a calendar file.
In my case it shows the entries of my iPhone reminder app.

I would be happy if someone reviews my code. I am especially
interested in comments about the use of kcalcore and the data engine.
But every other comment is welcome, too.


I just did a quick look over your code and tried to build it. You'll 
need to explicitly look for KdepimLibs + set include_dirs in order to 
build everywhere with no problems. Attached is a simple patch fixing 
the build.

Thank you very much for the patch and your comments.
Yesterday, I checked in your patch.

I tried running it but have no .ics files present so can't really try 
it out. Though your configuration dialog is somewhat too small and 
cannot be resized easily for some reason. One more comment on the 
visuals - if I add the applet is added on the desktop and you have no 
data/must set it up first, it should show some label what to do, 
otherwise it's just an empty Plasma-themed rectangle sitting on your 
desktop :)
I already noticed the issue with the configuration dialog, but couldn't 
figure out how to fix it, yet.

I will add the label with one of my next changes.


Final note, we do not really support Applets based on QGraphicsView 
anymore, the proper way to go is to use QML, which is way superior in 
tech and ease of writing. I think this[1] should be a good way to get 
you started :)

I will read the tutorial and decide afterwards, if I will change something.
I don't know much about QML, I thought it was only a specific 
programming language for QT development.

So, currently I don't understand the advantage. But we will see.



[1] - 
http://techbase.kde.org/Development/Tutorials/Plasma/QML/GettingStarted


Cheers
--
Martin Klapetek | KDE Developer


Best regards,

Conny
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Request for review

[Martin Klapetek]

Hey,

-- Forwarded message --
> From: Conny Marco Menebröcker 
> Date: Sat, Feb 15, 2014 at 7:44 PM
> Subject: Request for review
> To: kde-de...@kde.org
>
>
> Hello all,
>
>
> I have written my first plasmoid. I called it PlasmaTaskViewer,
> because it views the todos of a calendar file.
> In my case it shows the entries of my iPhone reminder app.
>
> I would be happy if someone reviews my code. I am especially
> interested in comments about the use of kcalcore and the data engine.
> But every other comment is welcome, too.
>

I just did a quick look over your code and tried to build it. You'll need
to explicitly look for KdepimLibs + set include_dirs in order to build
everywhere with no problems. Attached is a simple patch fixing the build.

I tried running it but have no .ics files present so can't really try it
out. Though your configuration dialog is somewhat too small and cannot be
resized easily for some reason. One more comment on the visuals - if I add
the applet is added on the desktop and you have no data/must set it up
first, it should show some label what to do, otherwise it's just an empty
Plasma-themed rectangle sitting on your desktop :)

Final note, we do not really support Applets based on QGraphicsView
anymore, the proper way to go is to use QML, which is way superior in tech
and ease of writing. I think this[1] should be a good way to get you
started :)

[1] -
http://techbase.kde.org/Development/Tutorials/Plasma/QML/GettingStarted

Cheers
-- 
Martin Klapetek | KDE Developer
Index: CMakeLists.txt
===
--- CMakeLists.txt  (revision 2)
+++ CMakeLists.txt  (working copy)
@@ -23,10 +23,11 @@
  
 # Find the required Libraries
 find_package(KDE4 REQUIRED)
+find_package (KdepimLibs REQUIRED)
 include(KDE4Defaults)
  
 add_definitions (${QT_DEFINITIONS} ${KDE4_DEFINITIONS})
-include_directories(
+include_directories(${KDEPIMLIBS_INCLUDE_DIRS}
${CMAKE_SOURCE_DIR}
${CMAKE_BINARY_DIR}
${KDE4_INCLUDES}
@@ -51,7 +52,7 @@
 target_link_libraries(plasma_dataengine_TaskViewerContainer 
${KDE4_PLASMA_LIBS} 
${KDE4_KIO_LIBS} 
-   kcalcore)
+   ${KDEPIMLIBS_KCALCORE_LIBS})
  
 install(TARGETS plasma_applet_taskviewer
 DESTINATION ${PLUGIN_INSTALL_DIR})
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: request for review

[Shaun Reich]

On Mon, Mar 19, 2012 at 6:28 AM, Sebastian Kügler  wrote:
> (Positive, because it works, negative, because I
> might have been looking for my furry porn collection, not something I'd want
> to tell any web services.)

yeah, good point. i don't want google knowing i'm into furry porn either.

as for what other people mentioned about the prefixes. i currently
have it activated by typing in "videos kde" and "images something". is
that acceptable? although i will be adding a flickr runner...so how
can i set it to be disabled by default?

-- 
Shaun Reich,
KDE Software Developer (kde.org)
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Re: request for review

[Shaun Reich]

On Mon, Mar 19, 2012 at 6:33 AM, Alex Fiestas  wrote:
> On Monday, March 19, 2012 11:28:22 AM Sebastian Kügler wrote:
>> Hey,
>>
>> One potential issue I see is that you ship the youtube logo inside the
>> runner. We might not be allowed to redistribute it as that.
>>
>> It also scales badly, so looks visually tarred.
>>
>> Can you check the license of the youtube logo, and if we're allowed to ship
>> it, make it look correct?
> I checked that back in the days when added youtube support to Kamoso, iirc you
> can use the youtube logo but without modifications (which makes sense). They
> also provide a policy on which logo you should use depending where you use it.


i checked it as well. from what i understood, you can can modify it as
long as it "still resembles the youtube logo". e.g. you can change the
background colors and stuff like that. they were pretty flexible on it
from what i gathered.

-- 
Shaun Reich,
KDE Software Developer (kde.org)
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Re: request for review

[Dario Freddi]

Il 19 marzo 2012 11:33, Alex Fiestas  ha scritto:
>> Also, enabling these runners by default, which you've essentially done,
>> brings a great privacy problem -- everything I type into KRunner is sent to
>> Google and Bing -- that's not acceptable as such, at least should be
>> discussed before we introduce this kind of change. I was pretty surprised
>> when I saw youtube movies in my krunner results. (Positive, because it
>> works, negative, because I might have been looking for my furry porn
>> collection, not something I'd want to tell any web services.)
>
> In theory they will only be triggered if the prefix match, kinda
>
> youtube: kde rocks

^ +1 to this. Having the (any) runner sending out anything I write on
the internet without my explicit consent is a no-go IMHO.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: request for review

[Aaron J. Seigo]

On Monday, March 19, 2012 11:33:13 Alex Fiestas wrote:
> In theory they will only be triggered if the prefix match, kinda
> 
> youtube: kde rocks

this is easy for the runner itself to do.

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Re: request for review

[Alex Fiestas]

On Monday, March 19, 2012 11:28:22 AM Sebastian Kügler wrote:
> Hey,
> 
> One potential issue I see is that you ship the youtube logo inside the
> runner. We might not be allowed to redistribute it as that.
> 
> It also scales badly, so looks visually tarred.
> 
> Can you check the license of the youtube logo, and if we're allowed to ship
> it, make it look correct?
I checked that back in the days when added youtube support to Kamoso, iirc you 
can use the youtube logo but without modifications (which makes sense). They 
also provide a policy on which logo you should use depending where you use it.

> Also, enabling these runners by default, which you've essentially done,
> brings a great privacy problem -- everything I type into KRunner is sent to
> Google and Bing -- that's not acceptable as such, at least should be
> discussed before we introduce this kind of change. I was pretty surprised
> when I saw youtube movies in my krunner results. (Positive, because it
> works, negative, because I might have been looking for my furry porn
> collection, not something I'd want to tell any web services.)

In theory they will only be triggered if the prefix match, kinda

youtube: kde rocks

If that can't be done for whatever reason then yes, we should disabled them by 
default (I already pushed that change a week ago).
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: request for review

[Sebastian Kügler]

Hey,

One potential issue I see is that you ship the youtube logo inside the runner. 
We might not be allowed to redistribute it as that.

It also scales badly, so looks visually tarred.

Can you check the license of the youtube logo, and if we're allowed to ship 
it, make it look correct?

Also, enabling these runners by default, which you've essentially done, brings 
a great privacy problem -- everything I type into KRunner is sent to Google 
and Bing -- that's not acceptable as such, at least should be discussed before 
we introduce this kind of change. I was pretty surprised when I saw youtube 
movies in my krunner results. (Positive, because it works, negative, because I 
might have been looking for my furry porn collection, not something I'd want 
to tell any web services.)

Cheers,
-- sebas

On Friday, March 16, 2012 13:10:54 Shaun Reich wrote:
> i recently merged some new runners into master kdeplasma-addons
> without a review and i apologize for that (the motivation actually was
> because i was having some difficulties making some packages and had a
> few repos that needed it, but that's moot now).
> 
> either way, i've got the tar if you've got the feathers ;-)
> 
> i can file a formal review request if that makes things easier. i will
> do so later for the non-merged duckduckgo runner, so as to not repeat
> the same mistake.
> 
> but the only things of interest here are the youtube and bing runners.
> the bing one is the same exact thing, except for bing obviously.
> 
> https://projects.kde.org/projects/kde/kdeplasma-addons/repository/revisions/
> master/entry/runners/youtube/youtube.cpp
> 
> https://projects.kde.org/projects/kde/kdeplasma-addons/repository/revisions/
> master/entry/runners/youtube/tubejob.cpp
> 
> the code's relatively straight forward, because json > xml ;)
> 
> the reason why i had to use bing for image searching is because google
> is evil and their api has some silly 1k queries/day/key limit, which
> we could easily outcap. and ironically, bing doesn't have any limits
> on their api. i'm probably going to work on a flickr runner as well,
> because we need more integration with the internets.
> 
> i do have a question however, as to why matches get triggered (even
> for the mediawiki engine which currently resides in there, afict),
> even though it isn't in singlerunnermode, and the prefix isn't being
> hit. e.g. user is typing in "some query" would trigger a match for
> that runner, instead of it only being triggered by "wiki some query".
> any ideas on that?
> 
> seems like a waste of resources if it is in fact unnecessary.
> 
> there is an issue with the icon being installed though. i copied the
> icons from some kipi plugin, because they also happened to have a
> youtube icon set so that saved time.
> 
> however, it now means we have 2 sets of icons being installed. also,
> i'm installing the icons simply by running: kde4_install_icons(
> ${ICON_INSTALL_DIR} )
> 
> is there a good fix for this?
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: request for review

[Shaun Reich]

> .. assuming i'm understanding what you're asking :)

yep, answers exactly that.



-- 
Shaun Reich,
KDE Software Developer (kde.org)
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: request for review

[Aaron J. Seigo]

On Friday, March 16, 2012 13:10:54 Shaun Reich wrote:
> i do have a question however, as to why matches get triggered (even
> for the mediawiki engine which currently resides in there, afict),
> even though it isn't in singlerunnermode, and the prefix isn't being
> hit. e.g. user is typing in "some query" would trigger a match for
> that runner, instead of it only being triggered by "wiki some query".
> any ideas on that?

because RunnerManager does not know anything about prefixes :)

with the RunnerSyntax there is a step in a useful direction there, but then 
there still needs to be a way saying "this runner only cares when the prefixes 
match" (some do both prefix matching and free text search)

in my testing (admitedly some time ago) there was very little benefit to doing 
this in RunnerManager however versus just letting the runners themselves figure 
it out.

.. assuming i'm understanding what you're asking :)

-- 
Aaron J. Seigo

signature.asc
Description: This is a digitally signed message part.
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel