Review Request: Adds Mobile Broadband info to plasma applet (depends on ModemManager patch)

[Lamarque Vieira Souza]

	Hi,

	This patch will not work without this other patch 
http://reviewboard.kde.org/r/3769 applied to solid. Since we are in hard 
feature freeze I cannot commit patch 3769. I will fix the other minor changes 
and when trunk is opened for new features I will commit both patches, until 
then I still have a lot of things to do in 3769.

	I did a test yesterday and Plasma::Label is capable of rendering a html-
like language used by Qt, that suffices to create the detail window. We could 
use only one Plasma::Label and add/remove its contents as needed. Updating the 
contents can make the screen flicker, is Plasma::WebView better in this 
regard? I would like to update only the needed parts, some parts (MAC, type, 
driver, etc) probably will not change, but others like signal quality can 
change anytime.

Em Quinta-feira 20 Maio 2010, Sebastian Kügler escreveu:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3778/#review5775
> -----------------------------------------------------------
> 
> Ship it!
> 
> 
> Looks fine, although we should change from a bunch of Plasma::Label's to a
> WebView, it's more flexible for this kind of stuff. See inline comments.
> There are also some minor things to fix up, so after doing this, you can
> commit this part.
> 
> Thanks, btw!
> 
> 
> /trunk/kdereview/networkmanagement/applet/interfacedetailswidget.h
> <http://reviewboard.kde.org/r/3778/#comment5427>
> 
>     private, please
> 
> 
> 
> /trunk/kdereview/networkmanagement/applet/interfacedetailswidget.cpp
> <http://reviewboard.kde.org/r/3778/#comment5429>
> 
>     Those guys should probably be hidden when we're not on a mobile
> broadband interface.
> 
>     I'd like to redo this, however:
> 
>     All details go into an HTML table in a webkit widget. That way we can
> assemble the information using conditional QString.append(). Theming will
> be done using Plasma::Theme::styleSheet().
> 
>     In order to save memory, the interfacedetailswidget needs to be deleted
> when it's not shown and recreated when details are viewed.
> 
>     That's well outside the scope of your patch, if you feel like doing
> that, please go ahead. If you don't the patch is fine as is (modulo the
> comments I've made), and you can commit it
> 
> 
> 
> /trunk/kdereview/networkmanagement/applet/interfacedetailswidget.cpp
> <http://reviewboard.kde.org/r/3778/#comment5428>
> 
>     whitespace (also in other places)
> 
> 
> - Sebastian
> 
> On 2010-05-20 02:10:05, Lamarque Souza wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://reviewboard.kde.org/r/3778/
> > -----------------------------------------------------------
> > 
> > (Updated 2010-05-20 02:10:05)
> > 
> > 
> > Review request for Network Management and KNetworkManager.
> > 
> > 
> > Summary
> > -------
> > 
> > Adds Mobile Broadband info to plasma applet details window. This patch
> > depends on http://reviewboard.kde.org/r/3769/ (Adds preliminary
> > ModemManager support to solid).
> > 
> > To test the patch firstly apply http://reviewboard.kde.org/r/3769/ to
> > kdebase-workspace, recompile solid and then recompile knetworkmanager
> > using this patch. You must logout and login again to the changes to take
> > effect. Some information are only available when using git
> > NetworkManager and ModemManager (May 16, 2010 at least).
> > 
> > 
> > Diffs
> > -----
> > 
> >   /trunk/kdereview/networkmanagement/applet/interfacedetailswidget.h
> >   1128399
> >   /trunk/kdereview/networkmanagement/applet/interfacedetailswidget.cpp
> >   1128703 /trunk/kdereview/networkmanagement/applet/nmpopup.cpp 1128609
> > 
> > Diff: http://reviewboard.kde.org/r/3778/diff
> > 
> > 
> > Testing
> > -------
> > 
> > 
> > Screenshots
> > -----------
> > 
> >   http://reviewboard.kde.org/r/3778/s/405/
> > 
> > Thanks,
> > 
> > Lamarque


-- 
Lamarque V. Souza
http://www.geographicguide.com/brazil.htm
Linux User #57137 - http://counter.li.org/
http://www.kde-mg.org