[Kde-hardware-devel] Review Request 121677: Fix mobile connection wizard manual apn setting

[Xuetian Weng]

> On Dec. 26, 2014, 10:46 a.m., Lamarque Souza wrote:
> > libs/editor/widgets/mobileconnectionwizard.cpp, line 159
> > <https://git.reviewboard.kde.org/r/121677/diff/1/?file=335846#file335846line159>
> >
> >     With your changes if mProvidersList->currentItem() is null here there will be a crash in "Confirm Page". That can happen if package mobile-broadband-provider-info is not installed or its files were manually deleted for instance. You should call radioManualProvider->setChecked(true) to prevent that.

At line 107, if the provider file couldn't be loaded, the dialog will close. After that the existence of the file doesn't matter since it's all in the memory.


> On Dec. 26, 2014, 10:46 a.m., Lamarque Souza wrote:
> > libs/editor/widgets/mobileconnectionwizard.cpp, line 185
> > <https://git.reviewboard.kde.org/r/121677/diff/1/?file=335846#file335846line185>
> >
> >     The crash is here. mProviders->currentItem() will be a null pointer.

Well, it's not caused by my patch, isn't?


> On Dec. 26, 2014, 10:46 a.m., Lamarque Souza wrote:
> > libs/editor/widgets/mobileconnectionwizard.cpp, line 206
> > <https://git.reviewboard.kde.org/r/121677/diff/1/?file=335846#file335846line206>
> >
> >     If you remove the separator in Plan's combobox remove this line too.

I only remove the separator if there's only "My plan is not listed...", in that case this branch will never be entered.


> On Dec. 26, 2014, 10:46 a.m., Lamarque Souza wrote:
> > libs/editor/widgets/mobileconnectionwizard.cpp, line 542
> > <https://git.reviewboard.kde.org/r/121677/diff/1/?file=335846#file335846line542>
> >
> >     And this one too. Have you really tested your changes? The way your patch is it will always select the plan before the correct one in the list.

After the execution of the if-else statement from line 149 to 169, there will always be at least 1 item in the combobox. It can be either
["My plan is not listed..."], or ["Default", separator, "Plan A", "Plan B", "My plan is not listed..."], no matter in which case, this code is totally fine.


- Xuetian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121677/#review72521
-----------------------------------------------------------


On Dec. 26, 2014, 7:46 a.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121677/
> -----------------------------------------------------------
> 
> (Updated Dec. 26, 2014, 7:46 a.m.)
> 
> 
> Review request for Solid and Lukáš Tinkl.
> 
> 
> Repository: plasma-nm
> 
> 
> Description
> -------
> 
> Current problems:
> 1. if user selects manual provider, apn text is not editable.
> 2. if user select "not listed" in apn page, and go back to provider page and go next again, apn text will be editable while it shouldn't.
> 3. a redundant separator if there's on "My plan is not listed...".
> 
> This change reuses slotEnablePlanEditBox function to set mApnText to correct state, and remove a separator if it's manually set provider.
> 
> 
> Diffs
> -----
> 
>   libs/editor/widgets/mobileconnectionwizard.cpp 29ce4f7 
> 
> Diff: https://git.reviewboard.kde.org/r/121677/diff/
> 
> 
> Testing
> -------
> 
> problems above are fixed.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20141226/0ec49e1d/attachment.html>