Moving plasma-nm to extragear

[Jan Grulich]

Hi,

thanks for the quick review.

On Friday 06 of June 2014 22:01 you wrote:
> El Dimarts, 27 de maig de 2014, a les 20:41:23, Jan Grulich va escriure:
> > Hi,
> > 
> > we would like to move plasma-nm to extragear/base and ask you for a
> > review.
> 
> Looks like in these i18n calls you should use proper argument 
placement and
> not "+"
> 
> return i18n("Connected") + ", ⬇ " + Download + ", ⬆ " + Upload;
> labelApn->setText("    " + i18nc("Mobile Connection Wizard", "APN") + ": " 
+
> userApn->text());
> labelApn->setText("    " + i18nc("Mobile Connection Wizard", "APN") + ": " 
+
> mApns.at(i));
> 

Yes, I'll fix it.

> 
> Looks to me that also it would be easier if you collapse the three ifs in a
> single condition (i couldn't find any difence between the three bodies of
> the ifs)
> 
> if (Type == PlasmaNM.Enums.Wired) {
> 	return i18n("Connected") + ", ⬇ " + Download + ", ⬆ " + 
Upload;
> } else if (Type == PlasmaNM.Enums.Wireless) {
> 	return i18n("Connected") + ", ⬇ " + Download + ", ⬆ " + 
Upload;
> } else if (Type == PlasmaNM.Enums.Gsm || Type == 
PlasmaNM.Enums.Cdma) {
>      return i18n("Connected") + ", ⬇ " + Download + ", ⬆ " + Upload;
> } else {
>      return i18n("Connected");
> }
> 

True. I expected different values/information for each type, so that's why it 
is separated.

> That's all i could find in a quick code review.
> 
> Now i'm using 0.9.3.3-0ubuntu1 on my desktop and i find that i'd like a
> scrollbar to signal there's more networks, otoh i know QML basically 
sucks
> scrollbar wise, so maybe adding a kind of fading/darkening anchored at 
the
> bottom of the listview viewport that is shown when we are not at the end 
to
> make it a bit more clear there's more stuff?
> 
> I know i'm asking a lot there so feel free to ignore this last suggestion :D
> 

I know it sucks without a scrollbar, but there is unfortunately no properly 
working component for that. Your solution would probably work fine, I could 
take a look when I have time, but I don't promise anything.

Btw. we already have a scrollbar in Plasma Next version.

Cheers,
Jan

-- 
Jan Grulich 
Red Hat Czech, s.r.o
jgrulich at redhat.com


> Cheers,
>   Albert
> 
> > Plasma-nm is a new applet written in QML for managing network 
connections
> > and contains a few parts:
> > 
> > 1) applet - plasma applet which allows you to connect/disconnect your
> > connections
> > 2) editor - application allows you to edit your network connections
> > 3) kded module - this is a secret agent managing passwords and
> > notifications 4) libs - plasma-nm libraries, like editor widgets, models
> > and so on 5) settings - KCMs for applet configuration
> > 6) vpn - contains all VPN plugins
> > 
> > You can find it here:
> > https://projects.kde.org/projects/kdereview/plasma-nm
> > 
> > Please review 0.9.3 branch, which is the current version for KDE 4.
> > Frameworks branch was merged to master today, so if you want to 
take a
> > look
> > at KF5/Plasma Next port, use master branch then.
> > 
> > Thanks a lot.
> > 
> > Cheers,
> > Jan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140607/35f4b5f8/attachment.htm>