Falkon in kdereview

[David Rosca]

On Sat, Mar 3, 2018 at 6:47 PM, Albert Astals Cid <aacid at kde.org> wrote:
> El dimecres, 28 de febrer de 2018, a les 12:10:36 CET, David Rosca va
> escriure:
>> Hi,
>> I'd like to request review for Falkon.
>>
>> It's been actually in kdereview for some time already, but I never got
>> to properly request review, sorry about that.
>>
>> There is a project set up in bugzilla, CI build and code should be in
>> accordance with guidelines too.
>> There are also some autotests, although they are rather unstable on
>> FreeBSD build. It looks like crash in QtWebEngine, but the backtrace
>> from CI is without symbols, so it is unfortunately useless.
>>
>> Target is Extragear for now, and later possibly moving to KDE Applications.
>
> Looks really nice :)
>
> One small fix you need to do, you need to also exclude scripts from your src/
> Messages.sh

Right, I missed that, thanks.

>
> the look of your scripts/Messages.sh file looks like it's trying to be too
> smart and it'll confuse some of our tools that may try to guess which .po
> files need packaging, i'd really suggest having one Messages.sh per subfolder,
> since after all you still need to edit the python_scripts variable so it's not
> automagic that any added script will get the .po extracted.

I did it this way so that the entire folder with python extension can
be installed with simple install(DIRECTORY ...) without excluding
anything. It seemed to work fine, but I'll change it so there is
Messages.sh in each subfolder.

>
> also two random comments, probably you can ignore most of them but since i
> spent some time i'll just comment.
>  * I think you need a qDeleteAll in AdBlockSubscription::loadSubscription
> before m_rules.clear();

Yes

>  * The bookmarks text in https://i.imgur.com/xkELczj.png doesn't fit here

It probably needs to remove titles completely, as they may be very
long depending on translation, and width of that sidebar should be
fixed.

David

>
> Cheers,
>   Albert
>
>>
>> Thanks,
>> David
>
>
>
>