ubuntu-touch-coreapps-reviewers team mailing list archive

[Renato Araujo Oliveira Filho <renato.filho@xxxxxxxxxxxxx>]

Thanks for reviewing it.

> OK, Let's have a review on it,
> 
> 1. perhaps the slot onParentDestroyed() on location.h lines 94-96 is not being
> used.
REMOVED


> 
> 2. on line 38 from networklistworker.cpp we should check if m_parent != 0,
>    the default value for 'parent' is 0 on networklistworker.h
> 
>    38: parent->connectDestructionSignal(this, SLOT(onParentDestroyed()));
FIXED

> 
> 3. are you sure the code from networklistworker.cpp lines 57-58 are really
> necessary?
>    the default value for 'parent' is 0 on networklistworker.h, and, if
> onParentDestroyed() is called
>    m_parent is set to 0 and the "m_parent != 0" is checked on line 61.
> 
>    57: if (!m_parent)
>    58:       return netContent;
> 
>    61: bool is_parent_of_smb_url = m_parent != 0 &&
> m_parent->urlPath().startsWith(LocationUrl::SmbURL);
> 
>    if you want keep lines 57-58, please change to "if (m_parent != 0)" as
> m_parent is not a boolean type.
This is not necessary (REMOVED)

> 
> 4. can you explain why you removed the call to "refreshInfo()" from
> Location::currentInfo()
>    on location.cpp line 329?
> 
>    I see that both Location::refreshInfo() and Location::becomeParent() delete
> the 'm_info',
>    this is the 'parent' used in NetworkListWorker class on the second thread.
>    That may be the real problem, perhaps just making an attribution into
> 'm_info'
>    from the temporary item could solve the entire problem, see a grep output:
> 
>      ./src/plugin/folderlistmodel/trash/trashlocation.cpp:57:
> delete m_info;
>      ./src/plugin/folderlistmodel/trash/trashlocation.cpp:176:        delete
> m_info;
>      ./src/plugin/folderlistmodel/location.cpp:64:        delete m_info;
>      ./src/plugin/folderlistmodel/location.cpp:96:        delete m_info;
>      ./src/plugin/folderlistmodel/location.cpp:238:        delete m_info;
>      ./src/plugin/folderlistmodel/location.cpp:252:            delete m_info;
> 
>    Would you consider (adding this) or reconsider  for this item #4 only?

We not not have a pointer to the temporary object (NetworkWorker) from Location class. Or do you have a way to access it?




-- 
https://code.launchpad.net/~renatofilho/ubuntu-filemanager-app/fix-network-crash/+merge/314870
Your team Ubuntu File Manager Developers is subscribed to branch lp:ubuntu-filemanager-app.