Merge lp:~gary-wzl77/scope-aggregator/force_display_results into lp:scope-aggregator

[Gary.Wang]

If only_in_search property of scope is set to true, aggregator will display the scope's results on search no matter if the scope is enabled or disabled in settings page.

If only_in_search property of scope is set to true, aggregator will display the scope's results on search no matter if the scope is enabled or disabled in settings page.

[Kyle Nitzsche (knitzsche)]

Hi Gary,

I am not sure I am comfortable showing results from child scopes where the user has disabled that child scope in settings. Can you please explain the rationale for this?

[Kyle Nitzsche (knitzsche)]

Hi,

If I understand correctly, this MR will cause results to be displayed from child scopes that are disabled by the user in settings. This seems incorrect to me. Can you please comment.

[Gary.Wang (gary-wzl77)]

Sorry for the late reply.
Yes, The main reason for this MP is for scope with only_in_search property
enabled, results will be only displayed on surface when searching. So
disable it in setting page will give user nothing if only this scope
support searching.Something, it looks odd, users have no ideas that results
from only_in_search scope will only be displayed when searching, so they
might think searching feature doesn't work in such case.

Generally, I think it would be better to hide scope with only_in_search
enabled in settings, which looks more reasonable.
But currently I can't find a way to do so.
Personally, I think it's a dirty fix as well. If you think it's invalid,
please reject it.

Thanks.

PS: This MP only affects only_in_search enabled scope, for generic scope,
it works as usual.

On Wed, Aug 19, 2015 at 2:32 AM, Kyle Nitzsche <email address hidden>
wrote:

> Hi,
>
> If I understand correctly, this MR will cause results to be displayed from
> child scopes that are disabled by the user in settings. This seems
> incorrect to me. Can you please comment.
> --
>
> https://code.launchpad.net/~gary-wzl77/scope-aggregator/force_display_results/+merge/267156
> You are the owner of lp:~gary-wzl77/scope-aggregator/force_display_results.
>

--
Br
Gary.Wzl

[Kyle Nitzsche (knitzsche)]

Hi Gary,

Is this functionality required for the BW scope (to work with a specific child scope we did not write)?

If this functionality is required then we should merge it because we do not want to fork scope-aggegator, and BW may need to be updated later from s-a new releases.

However, I think this whole "only_in_search" functionality is a work around (hack) that we needed for BW to work with a child scope we did not write (right?). If so, I doubt we will ever need this functionality again. Therefore: if you agree with this analysis, I propose that you modify the README and add a comment saying this json key is "deprecated and exists only to support a special case".

review: Needs Fixing

[Gary.Wang (gary-wzl77)]

Yes,
It's a workaround for a special case(Bollywood aggregator).
It doesn't support results search for child scopes in an old version,
except 3rd-party scope(cinema).
I've updated all other child scope to support query when the request comes
from aggregator.
It looks good so far.
Let's keep lean generic library and not add workarounds to make this
library fat.
I think that makes it easier for maintains as well.
Thanks.

On Fri, Aug 21, 2015 at 1:27 AM, Kyle Nitzsche <email address hidden>
wrote:

> Review: Needs Fixing
>
> Hi Gary,
>
> Is this functionality required for the BW scope (to work with a specific
> child scope we did not write)?
>
> If this functionality is required then we should merge it because we do
> not want to fork scope-aggegator, and BW may need to be updated later from
> s-a new releases.
>
> However, I think this whole "only_in_search" functionality is a work
> around (hack) that we needed for BW to work with a child scope we did not
> write (right?). If so, I doubt we will ever need this functionality again.
> Therefore: if you agree with this analysis, I propose that you modify the
> README and add a comment saying this json key is "deprecated and exists
> only to support a special case".
>
> --
>
> https://code.launchpad.net/~gary-wzl77/scope-aggregator/force_display_results/+merge/267156
> You are the owner of lp:~gary-wzl77/scope-aggregator/force_display_results.
>

--
Br
Gary.Wzl

[Kyle Nitzsche (knitzsche)]

Hi Gary,

Are you saying you do not want to merge this?

Cheers,
Kyle

[Gary.Wang (gary-wzl77)]

Yes. :)
Please reject it.
Thanks.

On Fri, Aug 21, 2015 at 10:46 PM, Kyle Nitzsche <<email address hidden>
> wrote:

> Hi Gary,
>
> Are you saying you do not want to merge this?
>
> Cheers,
> Kyle
> --
>
> https://code.launchpad.net/~gary-wzl77/scope-aggregator/force_display_results/+merge/267156
> You are the owner of lp:~gary-wzl77/scope-aggregator/force_display_results.
>

--
Br
Gary.Wzl

Unmerged revisions

146. By Gary.Wang on 2015-08-06

Display the scope's restuls on search
no matter if the scope is enabled or disabled in settings page.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-08-04 19:12:04 +0000
+++ CMakeLists.txt	2015-08-06 07:13:54 +0000
@@ -1,4 +1,4 @@
-set(VERSION "1.5.24")
+set(VERSION "1.5.24.1")
 
 # Supress qDebug() output
 ADD_DEFINITIONS( -DQT_NO_DEBUG_OUTPUT )

=== modified file 'README.md'
--- README.md	2015-07-15 03:34:23 +0000
+++ README.md	2015-08-06 07:13:54 +0000
@@ -718,6 +718,7 @@
 Value: string: "true" or "false", default value: "false"
 
 Displays only results of specified scope on search, not in surface.
+If only_in_search property of scope is set to true, aggregator will display the scope's restuls on search no matter if the scope is enabled or disabled in settings page.
 
 Note:If aggregator has multiple departments, in order to display results in current departments, you need to declare the scope with different deparment and local_id separately.
 

=== modified file 'src/query.cpp'
--- src/query.cpp	2015-08-04 19:12:04 +0000
+++ src/query.cpp	2015-08-06 07:13:54 +0000
@@ -332,7 +332,7 @@
                 }
             }
         }
-        if (!found_as_enabled)
+        if (!found_as_enabled && scope->only_in_search() == false)
         {
             qWarning () << QString("%1: scope is NOT ENABLED, skipping: %2").arg(qstr(SCOPE_ID), qstr(localId_id_m[local_id]));
             continue;