Proper rewrite for 4.1.1 and upwards

While developing NativeGodotRx as a GDExtension, I learned a lot of techniques to improve this project. The changes I would want to make in a future refactors:

Disposables dispose automatically when destroyed: This is a tough one. When I developed this extension, I was unable to properly pull this off. Using the 'self' reference during NOTIFICATION_PREDELETE always results in a Null Reference. I found a trick now. Assigning a member reference and then using unreference() gives us a behavior like std::enable_shared_from_this<>. This way, it is actually possible to keep the instance usable in GDScript
Proper thread management: See corresponding issue.
WeakKeyDictionary: See corresponding issue.  I see this as a bug.
Lock-Guards: I cannot see, why this is not a standard thing in GDScript. My god, it is awful to call lock() and unlock() every time we want to ensure thread-safety. Using the trick described earlier, I can now do them myself simplifying the code drastically.

Great news: I made a proper rewrite of GodotRx for Godot 4.1 fixing a lot of issues from the previous 4.0 version:

Disposables dispose when out of scope.
Locks now handled via LockGuards
WeakKeyDictionary fixed (increases subscription performance drastically)
ThreadManager introduced cleaning up all finished threads passively
Fixed memory leaks of ReactiveProperties/ReactiveCollections
Observable constructors now work with iterables (generators always possible now)
Fully committed to float as type for time units
Fixed inheritance madness of Subject, PeriodicScheduler and GodotSignalScheduler through redundancy (ugly but a lack of multi-inheritance has forced my hand)


Supported redmine 5.1.2 and add some feature

Hello, this PR has made the following changes to resolve some issues: #28

added support for Redmine 5.1.2.
added Chinese language configuration.
added a cookies parameter to the hook controller_account_before_cas_login.

Thank you for your contribution. I will merge it into the master branch. However, I would like to find a more efficient solution than manually copying the initializer to the config folder, as this step could easily be overlooked, especially during app updates to a new Redmine version.


Add bail on load errors

If Karma receives an error during the load of one of the plugins it currently swallows that error and continues running other plugins and exits with a zero error code.
I discovered this by investigating a successful build that didn't run any test on CircleCI. Turns out CI servers need a non-zero exit code to mark a build as failed.
Webpack 1.0 had a similar issue and they introduced a bail option that is turned on by default in 2.0 with a workaround npm package for 1.0 called webpack-fail-plugin. It would catch errors in plugins like babel that are used to compile the assets. A syntax error in the source code would cause babel plugin to fail and webpack would swallow the exception unless the bail option is on or fail plugin is present. See: https://github.com/webpack/webpack/issues/708
In the case of Karma the failure was in the PhantomJS npm package, where it could not locate the binary. It should have been a show stopper and reported a failure, but proceeded to load and execute the next plugins. See: https://github.com/karma-runner/karma-phantomjs-launcher/issues/120
This is my first attempt at looking at Karma, so please forgive my ignorance. This PR is a suggested solution and will need adjustments with your help.
Outstanding questions:

Should we fail by default?
Should we also introduce a bail option that is on by default? Or perhaps use another option?
Is this the right place to handle errors like this?

TODOs (would love help):

[ ] Write a test for a plugin load error

I signed it!

Travis build seems to be broken on master, let me know when it is fixed and I will rebase.  See here for details (sorry it is so buried - it is very easy to miss, and took me some drilling into to find this link).

@yagudaev Thanks!  I think a test for this issue is critical because we already are supposed to exit on load errors:
https://github.com/karma-runner/karma/blob/master/lib/server.js#L213
Something like karma/test/e2e/launcher-error.feature but load-error.feature would be awesome.

Sorry this did not work out. I'm going to close a lot of older pull requests as the source code has changed a lot over the last year.
Thanks!


[incubator-kie-issues#1460] Fix kogito-maven-plugin

Fixes https://github.com/apache/incubator-kie-issues/issues/1460
Needs:
https://github.com/apache/incubator-kie-kogito-apps/pull/2135
https://github.com/apache/incubator-kie-kogito-examples/pull/2032
This PR:

refactor generateModel mojo to internally call compile and process-classes
remove need to workarounded pom
remove usage of custom-lifecycle
remove need of extensions=true

Changes outside of the kogito-maven-plugin are only adaptation to the new implementation

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.
build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

PR job #2 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer
build-chain build full_downstream  -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3757 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3757/2/display/redirect
Test results:

PASSED: 3111
FAILED: 1

Those are the test failures:

org.kie.kogito.maven.plugin.it.kogito-maven-plugin-test.kogito-maven-plugin-test
The build exited with code 1. See /home/jenkins/jenkins-agent/workspace/_jobs_kogito-runtimes-pr_PR-3757/kogito-pipelines/bc/apache_incubator-kie-kogito-runtimes/kogito-maven-plugin/target/it/kogito-maven-plugin-test/build.log for details.

PR job #3 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer
build-chain build full_downstream  -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3757 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3757/3/display/redirect
Test results:

PASSED: 3463
FAILED: 1

Those are the test failures:

org.kie.kogito.maven.plugin.it.kogito-maven-plugin-test.kogito-maven-plugin-test
The build exited with code 1. See /home/jenkins/workspace/_jobs_kogito-runtimes-pr_PR-3757/kogito-pipelines/bc/apache_incubator-kie-kogito-runtimes/kogito-maven-plugin/target/it/kogito-maven-plugin-test/build.log for details.

PR job #4 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer
build-chain build full_downstream  -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3757 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3757/4/display/redirect
Test results:

PASSED: 3468
FAILED: 1

Those are the test failures:

org.kie.kogito.maven.plugin.it.kogito-maven-plugin-test.kogito-maven-plugin-test
The build exited with code 1. See /home/jenkins/jenkins-agent/workspace/_jobs_kogito-runtimes-pr_PR-3757/kogito-pipelines/bc/apache_incubator-kie-kogito-runtimes/kogito-maven-plugin/target/it/kogito-maven-plugin-test/build.log for details.

PR job #5 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer
build-chain build full_downstream  -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3757 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3757/5/display/redirect
Test results:

PASSED: 3468
FAILED: 1

Those are the test failures:

org.kie.kogito.maven.plugin.it.kogito-maven-plugin-test.kogito-maven-plugin-test
The build exited with code 1. See /home/jenkins/workspace/_jobs_kogito-runtimes-pr_PR-3757/kogito-pipelines/bc/apache_incubator-kie-kogito-runtimes/kogito-maven-plugin/target/it/kogito-maven-plugin-test/build.log for details.

@elguardian @fjtirado @pefernan @yesamer
After different attempts, I pushed a slightly different approach.
Context:

due to rule engine implementation, it is not possible to avoid a first compilation of the module before rules codegen
consequently, it does not make sense to spend time trying to fix that issue only for processes
the original scope of the ticket was to remove the needs of the extensions flag, as described in original description, but during iteration emerged also the request to avoid multiple compilations
the critical bit is the ProcessClassesMojo, that is responsible to codegenerate classes related to persistence, and uses reflections to generate json schema.

I tried to avoid compilation in the middle of the plugin execution, bringing around generated files and using JavaParser to read them.
Unfortunately, the last step is the generation of json schema, inside "ProcessClassesMojo", that currently is done by an external library (jsonschema-generator) that accept classes as input.
Re-implement this last step using sources would be extremely time-consuming and error-prone, so I stopped there.
But, as for the initial pre-compilation, if this step requires compiled classes, there is no reason to keep implementing "ProcessClassesMojo" from sources, instead of classes.
Then, the current PR has the following flow:

compilation of java classes present in the module
model code-generation based on model files
compilation of only the newly code-generated classes
code-generation of persistence classes and json schemas
compilation of only the newly code-generated classes

I also removed the ProcessClassesMojo and introduced some helper classes.
While this does not completely remove the needs of intermediate compilation (that would be extremely time-consuming and error-prone) at least

resolve the original issue of extensions flag
avoiding the usage of external library for hidden plugin invocation
avoid full recompilation in the middle of plugin execution
implement on-demand, only for needed source, compilation

Failure inside kogito-runtimes unrelated
 - apache_incubator-kie-kogito-runtimes/jbpm/jbpm-tests/src/test/java/org/jbpm/bpmn2/StandaloneBPMNProcessTest.java | expected: 2
 ##[error]expected: 2
 but was: 1

it is present also in unmodified/testing PR


[CXF-8025]: Fix Wrapper Style for multilevel xsd:extension elements

Original support was added in [CXF-2193].  This allows for multiple
levels of xsd:extension in elements while still using wrapper style.

Weird, that test passes for me locally.  I ran the full test suite with no failures on the cxf-3.3.1 tag and a few failures on master that were exactly the same with/without the patch (master from Tuesday).  This was on OSX with java 8 and maven 3.6.  Thoughts?

Don't worry, sometimes there are flanky tests on Jenkins. Thanks for your PR, looks good to me, but let's wait for another review before merging it.


Unable to create key in custom directory

while doing, autonomy keys add test --home  ~/.autonomy-1, the test key not stored in the custom home, it stored in the default home directory

This can be solved using keyring-dir flag


add shinyauthr to authentication section

Thanks for this great resource!
Adding my package that provides module functions that can be used to add an authentication layer to a shiny app.
Best regards,
Paul

-thanks @PaulC91! Good work. PR merged.
One small suggestion for shinyauthr is that I'd probably consider using slow hashes such as bcrypt instead of md5 to avoid offline attacks.

Thanks for the advice Nan! My knowledge of the infosec world is very limited so I really appreciate the feedback. I'll try and switch the hashing to bcrypt asap.


refactor: refactor component Popover and Tooltip

Refactor component Popover and Tooltip for code optimizing

Hey @hey-lee,
Awesome, I'll check this out today. Thanks for contributing!

Hey @hey-lee,
Sorry for being so late with the PR - we've made a lot of improvements to our components since, such as integrating TypeScript, instance management methods, and more. If you're still up to it you can fork it again and see what can be improved.
Cheers,
Zoltan


Raspberry Pi Authorisation Failure

Hi
After running the "sudo bash startsample.sh" at the Authorise and Run Stage I get the following message repeating time and time again:
#################################################
Checking for authorization (9)...
#################################################
2018-08-01 15:50:34.154 [  2] 5 HttpPost:doPostSucceeded:code=400
2018-08-01 15:50:34.158 [  2] 5 CBLAuthDelegate:receiveTokenResponse:code=400
2018-08-01 15:50:34.158 [  2] 5 CBLAuthDelegate:mapHTTPStatusToError:code=400,error=INVALID_REQUEST
2018-08-01 15:50:34.158 [  2] 5 CBLAuthDelegate:errorInLwaResponseBody:error=authorization_pending,errorCode=AUTHORIZATION_PENDING
2018-08-01 15:50:34.158 [  2] 5 CBLAuthDelegate:setAuthError:authError=AUTHORIZATION_PENDING
2018-08-01 15:50:34.158 [  2] 0 CBLAuthDelegate:receiveTokenResponseFailed:result=AUTHORIZATION_PENDING
2018-08-01 15:50:39.159 [  2] 5 CBLAuthDelegate:requestToken
Can somebody tell me what the problem is please?
Tell us what hardware you're using:

[ ] Desktop / Laptop
[ x] Raspberry Pi
[ ] Other - tell us more:

Tell us about your OS (Type & version):

[ ] Linux
[ ] MacOS
[x ] Raspbian Stretch
[ ] Raspbian Jessy
[ ] Other - tell us more:

Hi pgs32,
Could you refer to https://github.com/alexa/avs-device-sdk/issues/674 to fix this issue.
A short snippet of the fix
"Remove the CBL database file you are using. This will let the SDK prepare a fresh, uncorrupted database file.
Make sure the value of clientId in your AlexaClientSDKConfig.json file is the clientId found under the Other devices and platforms tab on developer.amazon.com, not the one found under the Web tab"
Thanks
Priya

I am closing this issue due to inactivity. Please feel free to re-open it if it has been closed in error.

How do you remove the CBL database?

Hi @jneubaum12 to remove the CBL database, search for "cblAuthDelegate" in the AlexaClientConfig.json file and remove the file under "databaseFilePath". Let us know if you face any issues with this. Thanks.


Add Config Sync resource related attributes to GCM

This change converts the non-k8s-pod typed resource attributes added in this PR that are related to Config Sync resources into metric labels.
The resource_filter in googlecloud exporter converts the cloud / k8s /
host related attributes directly into metrics labels in GCM.
Configsync.sync attributes are added with resource processor as metric
labels.
Adding metrics labels at reconciler level requires the shared Otel Collector
to remove them from all metrics before exporting to internal(Monarch) pipeline.
Labels are viewable in groupby drop down list and can be selected to
aggregate metrics.http://screen/ZtkKWoEx6bxnyiZ
This change also adds k8s.container.name to Config Sync metrics and Kustomize
wrapper metrics in Hydration Controller. This will make metrics to be mapped
to K8sContainer resource type. Change is compatible with current Monarch configuration.
Prometheus pipeline remain functioning http://screen/4tS9qSfoaFLSvAA
Context: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/issues/534

/retest-required

/hold

/retest-required

/unhold


Password field is mandatory in the back-end

As per earlier discussion, the password field was optional in the properties step. But as of now, it appeared to be a mandatory field in the backend.
@rk3rn3r @gunnarmorling please confirm if it's correct.
message: "Unable to connect: The server requested password-based authentication, but no password was provided." property: "database.hostname"

Closing this, it seems that field is still optional and it was checking/authenticating the already-set password(in tested scenario).


[TASK] Autoconnect should be enabled by default

Estimated story points: 1
Description

Acceptance criteria

time: 0.5h


Work around a types issue in webvr.

This bug is not in Typescript < 3.0, so we force a version between 2.0 and 3.0 to not get hit by this bug in ci. Once it is fixed we can update to > 3.* eventually again.
https://github.com/Microsoft/TypeScript/issues/26038

@wally-the-cartographer merge

Merge requested by authorized user SirVer. Merge queue now has a length of 1.

Error: PUT https://api.github.com/repos/googlecartographer/point_cloud_viewer/pulls/154/merge: 405 At least 1 approving review is required by reviewers with write access. []
Log:
=> handle work item: PR 154 on googlecartographer/point_cloud_viewer.
 => Switching to branch master.
 $ git reset --hard HEAD [data/point_cloud_viewer]
HEAD is now at efb6365 Refactor web viewer for reuse and rename it to be more distinct (#149)
 $ git clean -fd [data/point_cloud_viewer]
 $ git checkout master [data/point_cloud_viewer]
Already on 'master'
Your branch is up to date with 'origin/master'.
 $ git pull [data/point_cloud_viewer]
Already up to date.
Current branch master is up to date.
 => Checking out SirVer/00_force_typescript_29 as local branch pr_154.
 $ git remote add SirVer git@github.com:SirVer/point_cloud_viewer.git [data/point_cloud_viewer]
fatal: remote SirVer already exists.
 $ git fetch SirVer [data/point_cloud_viewer]
 $ git branch --track pr_154 SirVer/00_force_typescript_29 [data/point_cloud_viewer]
Branch 'pr_154' set up to track remote branch '00_force_typescript_29' from 'SirVer'.
 => Switching to branch pr_154.
 $ git reset --hard HEAD [data/point_cloud_viewer]
HEAD is now at efb6365 Refactor web viewer for reuse and rename it to be more distinct (#149)
 $ git clean -fd [data/point_cloud_viewer]
 $ git checkout pr_154 [data/point_cloud_viewer]
Switched to branch 'pr_154'
Your branch is up to date with 'SirVer/00_force_typescript_29'.
 $ git pull --force [data/point_cloud_viewer]
Already up to date.
Current branch pr_154 is up to date.
 => merge master.
 $ git merge master [data/point_cloud_viewer]
Already up to date.
 => Running clang-format on all *.cc and *.h files
 $ git commit -am Ran clang-format. [data/point_cloud_viewer]
On branch pr_154
Your branch is up to date with 'SirVer/00_force_typescript_29'.

nothing to commit, working tree clean
 $ git push --force SirVer HEAD:00_force_typescript_29 [data/point_cloud_viewer]
Everything up-to-date
 => Abandoning branch pr_154.
 => Switching to branch master.
 $ git reset --hard HEAD [data/point_cloud_viewer]
HEAD is now at 645f0e9 Another try.
 $ git clean -fd [data/point_cloud_viewer]
 $ git checkout master [data/point_cloud_viewer]
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
 $ git branch -D pr_154 [data/point_cloud_viewer]
Deleted branch pr_154 (was 645f0e9).

@wally-the-cartographer merge

Merge requested by authorized user SirVer. Merge queue now has a length of 1.


Apply filter to GPUImageView

I use GPUImageStillCamera to take a picture and load the picture into a GPUImageView
Now how can I apply a filter to a still GPUImageView?
The only examples I see online are to first create a GPUImagePicture (which requires a UIImage) but it seems rather wasteful to have to create a UIImage from GPUImageView then load it into the GPUImagePicture when I could just apply the filters to the GPUImageView directly. But I don't know how
I've tried:
GPUImageFilter *sepiaFilter [[GPUImageSepiaFilter alloc]init];
[sepiaFilter addTarget:gpuImageView];
//and then tried these in a bunch of different orders
[filter useNextFrameForImageCapture];
[currentFilter endProcessing];
[currentFilter imageFromCurrentFramebuffer];
Nothing works to apply the filter???

Put the filter in the pipeline between the camera and the GPUImageView. e.g.
filter = [allocate some filter]
[camera addTarget:filter]
[filter addTarget:yourGPUImageView]
If you then want to capture the result as a static image use:
[camera capturePhotoAsJPEGProcessedUpToFilter:filter withCompletionHander:^(NSData *processedImage, NSError *erro){}];
In your completion handler block you can convert the NSData to an image or do what you wish with the image data.
Thanks,
Neil Schreiber
<EMAIL_ADDRESS>847 275 6308

On Jan 23, 2015, at 12:05 PM, MobileMon<EMAIL_ADDRESS>wrote:
I use GPUImageStillCamera to take a picture and load the picture into a GPUImageView
Now how can I apply a filter to a still GPUImageView?
The only examples I see online are to first create a GPUImagePicture (which requires a UIImage) but it seems rather wasteful to have to create a UIImage from GPUImageView then load it into the GPUImagePicture when I could just apply the filters to the GPUImageView directly. But I don't know how
I've tried:
GPUImageFilter *sepiaFilter [[GPUImageSepiaFilter alloc]init];
[sepiaFilter addTarget:gpuImageView];
//and then tried these in a bunch of different orders
[filter useNextFrameForImageCapture];
[currentFilter endProcessing];
[currentFilter imageFromCurrentFramebuffer];
Nothing works to apply the filter???
—
Reply to this email directly or view it on GitHub https://github.com/BradLarson/GPUImage/issues/1892.

@neils4fun the problem is I don't want to convert the NSData to a UIImage just to apply a filter as its too memory intense
After I capture the photo into a GPUImageView (which is what I do in like your above example), I want to apply another filter to the still image (Just like Instagram where you tap a bunch of different filters and are applying them to a still image)
I want to apply the filter directly to the GPUImageView not first convert to UIImage then load UIImage back into GPUImageView

ok then to the best of my knowledge at that point you do need to
Thanks,
Neil Schreiber
<EMAIL_ADDRESS>847 275 6308

On Jan 23, 2015, at 3:29 PM, MobileMon<EMAIL_ADDRESS>wrote:
@neils4fun https://github.com/neils4fun the problem is I don't want to convert the NSData to a UIImage just to apply a filter as its too memory intense
After I capture the photo into a GPUImageView (which is what I do in like your above example), I want to apply another filter to the still image (Just like Instagram where you tap a bunch of different filters and are applying them to a still image)
I want to apply the filter directly to the GPUImageView not first convert to UIImage then load UIImage back into GPUImageView
—
Reply to this email directly or view it on GitHub https://github.com/BradLarson/GPUImage/issues/1892#issuecomment-71260191.


Explain on Tutorials page that contributions are more than welcome

Migrated from GitLab: https://gitlab.com/meltano/meltano/-/issues/2535
Originally created by @DouweM on 2021-01-25 21:48:02

View 2 previous comments from the original issue on GitLab


Git db compression fails

Issue:
Release train workflow failed. the traces give out git.repack and lead to maybeGc
https://app.datadoghq.eu/logs?query=service%3Akuberpult-cd-service status%3Aerror &additional_filters=[{}]&cols=host%2Cservice&index=*&messageDisplay=inline&spanID=7196714735387314404&stream_sort=time%2Cdesc&viz=stream&from_ts=1676369423529&to_ts=1678961423529&live=true
Possible reasons:
maybeGc is running the git db compression.
This compression takes extra resources. That could be too much.

https://github.com/freiheit-com/kuberpult/blob/f973533c179a846a000a23c4a0177ec41a493e26/services/cd-service/pkg/repository/repository.go#L659-L676

Expected behavior:
kuberpult requests don't fail because of this compression.
solutions

maybe run the compression periodically? in a less busy time?
run the compression more often?
remove the compression altogether? and do more pod restarts?

We have a story for this (just talked about this with Hannes) https://revolution.dev/app/-JqFGExX46gs9mH7vxR5/-M37cZx7XYoOvQ5Mmcir/STORY/-NR2eQPxASwlooQBhXIe/


godot server build crash at export

That is on Linux. I tried to export project. The cresh is due to CameraServer not available.
The backtrace:
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff7fcf700 (LWP 1405)]
Godot Engine v3.2.dev.custom_build.364fbe89e - https://godotengine.org
[New Thread 0x7ffff75c9700 (LWP 1406)]
[New Thread 0x7ffff7588700 (LWP 1407)]

Thread 1 "godot_server.x1" received signal SIGSEGV, Segmentation fault.
0x000055555844a458 in CowData<Ref<CameraFeed> >::_get_size (this=0x100) at ./core/cowdata.h:70
70			if (!_ptr)
(gdb) bt
#0  0x000055555844a458 in CowData<Ref<CameraFeed> >::_get_size (this=0x100) at ./core/cowdata.h:70
#1  0x0000555558449f66 in CowData<Ref<CameraFeed> >::size (this=0x100) at ./core/cowdata.h:125
#2  0x000055555844994a in Vector<Ref<CameraFeed> >::size (this=0xf8) at ./core/vector.h:83
#3  0x0000555558447f50 in CameraServer::get_free_id (this=0x0) at servers/camera_server.cpp:69
#4  0x000055555855ce9f in CameraFeed::CameraFeed (this=0x55555ae43960) at servers/camera/camera_feed.cpp:138
#5  0x00005555584d6dc4 in ClassDB::creator<CameraFeed> () at ./core/class_db.h:146
#6  0x000055555879260e in ClassDB::instance (p_class=...) at core/class_db.cpp:540
#7  0x0000555558797136 in ClassDB::class_get_default_property_value (p_class=..., p_property=..., r_valid=0x7fffffff97cf) at core/class_db.cpp:1415
#8  0x000055555749a90c in DocData::generate (this=0x55555acbb120, p_basic_types=true) at editor/doc/doc_data.cpp:255
#9  0x00005555571afca0 in EditorHelp::generate_doc () at editor/editor_help.cpp:1421
#10 0x00005555572185a3 in EditorNode::EditorNode (this=0x55555acba520) at editor/editor_node.cpp:5306
#11 0x00005555566a5239 in Main::start () at main/main.cpp:1624
#12 0x0000555556679cf2 in main (argc=6, argv=0x7fffffffe168) at platform/server/godot_server.cpp:42

See https://github.com/godotengine/godot/commit/e30a137548dce0fe1f5eda223b900fcbdd2631b8

Thanks, this is resolved now via https://github.com/godotengine/godot/commit/e30a137548dce0fe1f5eda223b900fcbdd2631b8

(For the record, this issue was a duplicate of #30630.)


SecurityContext (HttpServer.bindSecure) with multiple hosts.

I couldn't find any example, test or documentation on how to create a https server with multiple hostnames (separate certificates for each hosts). Is is supported?

cc @jonasfj @sortie

Maybe if you concatenate the certificates.... You can certainly do multiple hostnames in a single certificate. Regardless, I think this is a pretty advanced feature.

But really, I would recommend terminating TLS before you hit your Dart server.

Any example in how to do build a SecurityContext  with multiple domains?
Or how to concatenate the certificates? I tried some variations here, but I can't make it work.

Also, relevant and useful package on pub.dev, with @gmpassos as the author, which could really use this multi-host binding: https://pub.dev/packages/shelf_letsencrypt

To generate a PKCS12 with multiple domains certificates generated with the same key pair (same private key) and with the password xyz:
openssl pkcs12 -export \
-inkey /path/to/privkey.pem \
-in /path/to/domain1.com/fullchain.pem -name domain1.com \
-certfile /path/to/domain2.com/fullchain.pem -name domain2.com \
-out /path/to/keystore-multi.pfx -passout pass:xyz

To generate aPKCS12 with multiple domains certificates WITHOUT add a private key and with the password xyz:
openssl pkcs12 -export \
-nokeys \
-in /path/to/domain1.com/fullchain.pem -name domain1.com \
-certfile /path/to/domain2.com/fullchain.pem -name domain2.com  \
-out /path/to/keystore-multi.pfx -passout pass:xyz

The Dart code to load this:
var password = 'xyz';
var fullchainFile = '/path/to/keystore-multi.pfx';
var privateKeyFile = '/path/to/privkey.pem';

var sc = SecurityContext()
  ..useCertificateChain(fullchainFile, password: password)
  ..usePrivateKey(privateKeyFile, password: password);

Note that if you use a private key that won't match the certificates you will get an exception while loading the certificates.
The only way that I was able to load multiple certificates were using multiple domains certificates generated with the same private key (yes this is allowed).
But this is still not enough, since the SecurityContext will always load the 1st domain in the PKCS12 file!
It seems that the Dart implementation is not looking for the correct certificate subject (subject=CN = domainX.com).

FYI:
https://github.com/dart-lang/sdk/blob/e995cb5f7cd67d39c1ee4bdbe95c8241db36725f/runtime/bin/security_context.cc#L538

FYI:
https://github.com/dart-lang/sdk/blob/e995cb5f7cd67d39c1ee4bdbe95c8241db36725f/runtime/bin/secure_socket_filter.cc#L614

@mit-mit @kevmoo
We need a task force for this issue. This is very important.

FYI @brianquinlan

@brianquinlan ?

How can I help to push this issue?

Hi, any improvement?
It's all about the selection of the correct certificate (server side) for the requested URL/domain.

?

How can I help to push this issue?

Up. Please, this is extremely important!!

I'm interested in this too

FYI: @kevmoo @brianquinlan
I’ve published a pure Dart solution for this issue:
https://github.com/gmpassos/multi_domain_secure_server
Additionally, there’s an example demonstrating its use with shelf:
https://github.com/gmpassos/multi_domain_secure_server/tree/master/example/shelf_example
Any feedback would be greatly appreciated!
Best regards,

@gmpassos Thank you, this looks interesting and useful! I shall try it out in the coming weeks.

FYI: @Martoxdlol @marcobraghim @isoos
I'm looking for some beta testers before releasing the final version!
Usage
shelf_letsencrypt: ^2.0.0-beta.7

shelf_letsencrypt on GitHub

Your feedback would be greatly appreciated! 🚀

FYI: @Martoxdlol @marcobraghim @isoos
I'm looking for some beta testers before releasing the final version!
Usage
shelf_letsencrypt: ^2.0.0-beta.7

shelf_letsencrypt on GitHub

Your feedback would be greatly appreciated! 🚀

Cool. Sorry I missed it but this is really interesting!


Property DDS: Single property with inheritance

Issue Category
Can't find an answer to a specific question
Details
It is unclear if using inheritance on a single field is supported and how it is done.   Pull Request Property Docs #6104  covers a lot in this area, but does not seem to directly address this use case.
The problem is probably best illustrated with a test case.  See last block of comments
import {
    PropertyFactory,
    MapProperty,
    ContainerProperty,
    ArrayProperty,
} from "@fluid-experimental/property-properties";

describe("Test inheritance use cases", function () {
    const TYPE_STRUCT = {
        typeid: "test:Struct-1.0.0",
        properties: [{ id: "value", typeid: "String" }],
    };

    const TYPE_STRUCT_SUBTYPE = {
        typeid: "test:StructSubType-1.0.0",
        inherits: [TYPE_STRUCT.typeid],
        properties: [{ id: "value2", typeid: "String" }],
    };

    const TYPE_ROOT = {
        typeid: "test:Test-1.0.0",
        properties: [
            { id: "fieldSingle", typeid: TYPE_STRUCT.typeid, context: "single" },
            { id: "fieldArray", typeid: TYPE_STRUCT.typeid, context: "array" },
            { id: "fieldMap", typeid: TYPE_STRUCT.typeid, context: "map" },
        ],
    };

    PropertyFactory.register([TYPE_STRUCT, TYPE_STRUCT_SUBTYPE, TYPE_ROOT]);

    test("set single field with subtype", async () => {
        const root = PropertyFactory.create(
            TYPE_ROOT.typeid
        ) as ContainerProperty;

        // Array supports inheritance
        const array = root.get("fieldArray") as ArrayProperty;
        const valueForArray = PropertyFactory.create(
            TYPE_STRUCT_SUBTYPE.typeid,
            "single"
        ) as ContainerProperty;
        array.push(valueForArray);

        // Map supports inheritance
        const map = root.get("fieldMap") as MapProperty;
        const valueForMap = PropertyFactory.create(
            TYPE_STRUCT_SUBTYPE.typeid,
            "single"
        ) as ContainerProperty;
        map.insert("test", valueForMap);

        // How to set `root.fieldSingle` to a subtype of test:Struct-1.0.0?
        // The Property DDS API seems not to support inheritance when it comes to single fields
        const single = root.get("fieldSingle") as ContainerProperty;

        const valueForSingle = PropertyFactory.create(
            TYPE_STRUCT_SUBTYPE.typeid,
            "single"
        ) as ContainerProperty;

        const fieldValues = valueForSingle.getValues();
        expect(() => single.setValues(fieldValues)).toThrowError(
            "PR-116: trying to set value to an invalid path: value2"
        );
    });
});

@taylorsw04, can you please glance?

While using PropertyDDS more it's become clear that single properties do not support inheritance.  When it's needed, the best option appears to use an array and rely on application code to avoid having an array length greater than one.  Using an array in this way gives us the benefit supporting a null value by allowing the array to be zero-length (another scenario not support by single)
I'm ok to close, if you're ok

Yup, seems reasonable to me. Thanks!


Have more verbose test failure information in testTimeout

testTimeout is flaky (example) but the error information is not verbose enough to diagnose. This PR is to allow Kokoro to generate more useful information for me to debug the problem.

Test failures not related to ObjC change.


Add data prefiltering to catch various evasion techniques

This code is primarily forklifted with permission from the PHPIDS project. Added a specific urldecode method to take care of issue #31. This does not include the PHPIDS "centerfuge" which is a bit resource heavy (and I have not dug to heavily into). This patch raises the sensitivity level of numerous vectors to highlight their malicious intent.

Should be good to go. Added the spaces around = into a separate merge. @enygma , could you flesh out the Converter suggestion in a ticket?


Allow default patches with go_deps

The go_deps extension has a mechanism for having different default arguments for the underlying go_repository command. Currently there are overrides for build_directives, build_extra_args, and build_file_generation: https://github.com/bazel-contrib/bazel-gazelle/blob/28f43a444ecc4c0498928787ebb1e61fef08f6eb/internal/bzlmod/go_deps.bzl#L18-L20
The code is written in a way that we one could specify new defaults, but this is currently not done: https://github.com/bazel-contrib/bazel-gazelle/blob/master/internal/bzlmod/go_deps.bzl#L156-L160
The go-tree-sitter library is used by the gazelle plugin for rules_python to parse python files during build file generation, and needs to be upgraded to a new version to support python 3.12 type parameter syntax (see https://github.com/bazelbuild/rules_python/issues/2396). However, the new version of go-tree-sitter does not build with gazelle because it has uplevel references to c header files. We tried to fix this by applying patches in the rules python repo (see https://github.com/bazelbuild/rules_python/pull/2413), but those overrides don't take effect when using using the rules python gazelle plugin because it is no longer the root module, so we would need the patches to be applied by bazel-gazelle.

Note that if gazelle could handle uplevel file references when generating build files then patches for go-tree-sitter would not be necessary, though it may still be desirable to have a mechanism for applying default patches.

I understand your use case, but maintaining patches to Go modules in this repo would certainly overstretch our responsibility budget. For example, we would need to have a plan for when the patch needs to change as new versions of the module are released.
Have you considered making go-tree-sitter a BCR module instead? Gazelle has special support for BCR modules that are also Go modules and avoids duplicating them in the dependency graph. You can refer to the circl module for an example.


refactor: change jest rule's category

Current dependencies on/for this PR:

main

PR #1155   👈

This stack of pull requests is managed by Graphite.

You need to run gt ss 😅

Done, forgot to create a new branch🙃

Done, forgot to create a new branch🙃

You don't need to create branches, just gt c and then gt ss.


[SPARK-43055][CONNECT][PYTHON][FOLLOWUP] Fix deduplicate field names and refactor

What changes were proposed in this pull request?
Fixes deduplicate field names, and refactor to use the same renaming rule between ArrowTableToRowsConversion.convert and LocalDataToArrowConversion.convert.
Why are the changes needed?
If there is a duplicated field name in a separate position, it fails to deduplicate and returns a wrong result.
>>> from pyspark.sql.types import *
>>> data = [
...     Row(Row("a", 1), Row(2, 3, "b", 4, "c", "d")),
...     Row(Row("w", 6), Row(7, 8, "x", 9, "y", "z")),
... ]
>>> schema = (
...     StructType()
...     .add("struct", StructType().add("x", StringType()).add("x", IntegerType()))
...     .add(
...         "struct",
...         StructType()
...         .add("a", IntegerType())
...         .add("x", IntegerType())
...         .add("x", StringType())
...         .add("y", IntegerType())
...         .add("y", StringType())
...         .add("x", StringType()),
...     )
... )
>>> df = spark.createDataFrame(data, schema=schema)
>>>
>>> df.collect()
[Row(struct=Row(x='a', x=1), struct=Row(a=2, x=None, x=None, y=4, y='c', x=None)), Row(struct=Row(x='w', x=6), struct=Row(a=7, x=None, x=None, y=9, y='y', x=None))]

It should be:
>>> df.collect()
[Row(struct=Row(x='a', x=1), struct=Row(a=2, x=3, x='b', y=4, y='c', x='d')), Row(struct=Row(x='w', x=6), struct=Row(a=7, x=8, x='x', y=9, y='y', x='z'))]

Does this PR introduce any user-facing change?
No.
How was this patch tested?
Updated the related test.

Merged to master.


Convert enums to Qt6 style

Description of Changes
In Qt6 the enums are no longer located directly under QtCore.Qt. This is fully compatible with Qt5, so a small step towards easier Qt6 support.
This would convert most of them (probably some are still missing).
I went with separate commits to make it easier to review, but let me know if it is preferred to squash them.
(I do not understand what happened with scripts/spyder.bat. I did my best to revert the "change" locally, but for unknown reasons, my git client thought that it was modified...)

[ ] Wrote at least one-line docstrings (for any new functions)
[ ] Added unit test(s) covering the changes (if testable)

[ ] Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
oscargus/Oscar Gustafsson

I realize that I should have kept external-deps out of it... Problem with automatic search and replace...

Checking I think QtPy should be taking care of this with the enums promotion to allow unscoped access even with PyQt6 at https://github.com/spyder-ide/qtpy/blob/a94438f50942c23031776d8698883661eeb8e954/qtpy/QtCore.py#L70-L75
Thinking, maybe this be related with https://github.com/spyder-ide/qtpy/issues/364 ? From what I have been able to check seems like PyQt6 is the only binding that dropped support for unscoped enum access. Maybe @oscargus do you have more info about the state of that or after which version the Qt6/PySide6 projects will be dropping support for unscoped enums access?


Added check_key in RedisCache incr

Trac ticket number
N/A
Branch description
In redis, incr can don't need check key, so add check_key to this command. The defauult value is true
Checklist

[y] This PR targets the main branch.
[y] The commit message is written in past tense, mentions the ticket number, and ends with a period.
[ ] I have checked the "Has patch" ticket flag in the Trac system.
[y] I have added or updated relevant tests.
[y] I have added or updated relevant docs, including release notes if applicable.
[y] For UI changes, I have attached screenshots in both light and dark modes.

I think that the fundamental problem here is that cache.incr / decr are documented to raise an exception if they key is missing which I believe is a bit strange in retrospective given how often this method is used in the wild to perform moving window throttling.
It means that practically every call to it must be paired with a either a systematic add
if not cache.add(key, delta):
    cache.incr(key, delta)

Or be performed as a fallback
try:
    cache.incr(key, delta)
except ValueError:
    if not cache.add(key, delta):
        cache.incr(key, delta)

Add to that the underlying exists call performed by the Redis backend and it means that it order to use incr properly there is no way around doing
SET key delta NX
--- Do the following in most cases
EXISTS key
INCRBY key delta

or
EXISTS key
INCRBY key delta
--- Do the following in fallback cases
SET key delta NX
EXISTS key
INCRBY key delta

That's up to 3-5 operations which are non-pipelined (meaning they'll each need to roundtrip over the network serially) and non-atomic. Things is this could all be done in a single INCRBY key delta assuming incr and decr supported a way to specify a fallback to pre-existing value (which I assume 0 would be the case most of the time anyway).
I focused on Redis here but from eye balling all the other existing backend it seems it wouldn't be hard to adapt them to augment the interface to accept a kwarg (alike to the check_key proposed here) so it simply assigns delta instead of erroring out when a key is missing. This seems more valuable and likely to be accepted that hacking a solution solely for the Redis backend.
I suggest that you raise this point on the forum @cyx2000 as documented for new features proposal of this type to gather feedback.
I'll close this MR in the meantime as it's not in a receivable form due to the lack of consensus on the proper solution.

Thanks for your reply and review.


Check the responsiveness of login page in mobile view

@QAZIMAAZARSHAD can I work on this issue under LGM-SOC??

Can I work on this issue?

Issue resolved


net: openthread: Update to the latest revision

Update the OpenThread repository to its latest revision.
The most significant changes that impact the Zephyr port are CLI and NCP API changes. The CLI update was pretty straightforward, but for the NCP the changes are more significant.
The NCP module no longer use the UART APIs (which are no longer part of the OT platfrom abstraction layer) but instead provides its own handler to send data and callbacks to report TX/RX completion. For the sake of simplicity of the upmerge, these new APIs are just thin wrappers around the exisiting UART API (just as the upstream NCP example does) but this leaves an opening to re-thing the UART component API in the future (and possibly simplify it).

CC @MarekPorwisz

Am I missing something, I can't seem to find utils/uart.h anywhere?


.Net: MinHeap and TopNCollection should not be public

[!IMPORTANT]
Labeled Urgent because it may require a breaking change.

https://github.com/microsoft/semantic-kernel/issues/615 was closed as completed with (I thought) the resolution to make MinHeap internal, but in the latest release it's still public.  Both it and TopNCollection aren't something that are really in the domain of SK: a developer can use PriorityQueue<> in .NET, or OrderBy(...).Take(...), or any number of other solutions if they need the equivalent.

@stephentoub We moved them to separate package Microsoft.SemanticKernel.Plugins.Memory and made them public as part of this package. One of the reasons is to be able to write some unit tests in separate test package. Otherwise, we will need to add InternalsVisibleTo attribute, and I'm not sure it's a good practice for testing internal classes/structures.
Do you have any recommendations here?

What are the unit tests doing with MinHeap/TopNCollection that they can't do with PriorityQueue, OrderBy.Take, etc.?

What are the unit tests doing with MinHeap/TopNCollection that they can't do with PriorityQueue, OrderBy.Take, etc.?

Sorry, I meant unit tests for actual MinHeap/TopNCollection types :)
https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/Plugins/Plugins.UnitTests/Memory/Collections/MinHeapTests.cs
https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/Plugins/Plugins.UnitTests/Memory/Collections/TopNCollectionTests.cs
TopNCollecton is used in VolatileMemoryStore - our in-memory store implementation. Since Plugins.Memory package is based on .NET Standard 2.0, PriorityQueue is unavailable, unless we copy all its source code and place it in our repository.

Makes sense, thank you! Will use InternalsVisibleTo flag then.


Improve synonyms functions

What the pull request does
Add methods to work on synonyms
Type of change

[x] :sparkle: New feature (non-breaking change which adds functionality)
[x] :book: This change requires a documentation update

Checklist

[x] The changes are documented

[x] The docstrings of the different functions describe the arguments, purpose, and behavior
[x] There are examples in the documentation website

[x] The changes are tested
[x] The changes do not modify the behavior of the previously existing functions
[x] The Project.toml field version has been updated

Codecov Report

Merging #28 (919f03c) into main (0859f41) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #28   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files            8         8
  Lines           83        87    +4
=========================================
+ Hits            83        87    +4

Flag
Coverage Δ

unittests
100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

src/NCBITaxonomy.jl
100.00% <ø> (ø)

src/nametools.jl
100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0859f41...919f03c. Read the comment docs.


Update microsoft-365-copilot.md

Adding missing Licensing for Semantic Index

Taking discussion offline with submitter.


Please support Docker DB 21.3 - Apple Silicon

Understood that this is a known issue.
Any update on what this would be supported?  The dev community is really adopting Apple Silicon as the dev platform of choice.  Thanks!

There are ongoing discussions of workarounds in https://github.com/oracle/docker-images/discussions/1951 and as you state, this is a known issue. Closing this issue in favour of the existing discussion.


Image API missing

It seems like Tern has most of the browser/DOM API built in but is missing support for Image.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/Image
Edit:
Actually it seems like Tern has most browser elements included in the definition file, but there is very little specialized information about each element, e.g the HTMLImageElement is missing src and alt. They are all of the type Element and they all contain the same information. Is there any way to inherit the prototype from Element, but specialize the prototype in the json definition files? I'd like to add src, onload, onerror to the HTMLImageElement and also add a constructor for Image() that returns a new HTMLImageElement.
I've made a PR to include the Image() constructor in #616

Fixed via #616

@marijnh Is it possible to have specific attributes on the HTMLImageElement while extending from Element in a definition file? Can you show a quick example?

Tern treats all Elements as being of the same type. The reason for this is that most sources of Element values (childNodes, querySelector, parentElement, etc) don't distinguish between the various sub-types of Element or Node, and thus, if we moved things like src and getContext to specific (image or canvas) elements, we'd lose completion and typing for them in 99% of cases.

Ah, makes sense.
However if I were to write my own little library and had an inheritance chain, how could I write definitions for that? Can type A inherit from type B and still have something specific in it's prototype?

Yes. You can see for example the way Element inherits from Node in browser.json -- its prototype object has a "!proto": "Node.prototype" declaration.

Ah! Thanks!

@dani-h I have written https://github.com/angelozerr/tern-browser-extension whioch extends browser definition to use it inside HTML and gives extra features like elements ids (document.getElementById), CSS selectors (for document.querySelectors) and document.getElementById, document.createElement is able to returns the well instance of Element. So we could support your case :
<img id="MyImg"" src="..." />
<script>
var img  =document.getElementById("MyImg");
img. // here completions shows src

To do that I must overrid eteh original browser.json to had src inside HTMLImgElement inside preLoadDef https://github.com/angelozerr/tern-browser-extension/blob/master/browser-extension.js#L320
If you are nterested, any contribution are welcome!

@angelozerr Looks nice at first glance. I'll try it out. Does it only support inline scripts?

Pay attention, I have not tested with the ternjs in the master (I will do that soon).

Does it only support inline scripts?

You mean JavaScript inside on* attributes? If it that it support it, but this is not managed.

No, all of your examples on the page display scripts in lined with HTML. Does it support standalone js files that are loaded?

You mean have completion for HTML element ids inside a script a.js which is included inside an HTML file. If it that, no, but I think we could support it (by showing teh whole ids of the HTML files).
I suggest you that you create issues at https://github.com/angelozerr/tern-browser-extension/issues to avoid disturbing tern project.


Add support for showing Poll Widgets.

Widgets support has been in great demand from users (as it was
implemented in the webapp first, and we haven't supported its
display since support was added to the API).
This PR adds the poll widget support, and modularises widgets
so it will be easier to add other widgets if required. It uses the
newly introduced (c174a0) getOwnUser to get the user_id of the
current user in order to check it against the key provided in
submessages (which are only widgets for now) of type vote, to see
if the current user has voted on the option in that poll.
Styling has been duplicated from and merged with .reaction, while
the borders of the .widget class has been changed to match the
.static-timestamp class. You will also see a Poll Widget marker in the
bottom right of the box which shows it.
Fixes #3205 .

Thanks @ishammahajan!
I just played with this a bit in the app, and with a different user in the webapp, on the same poll. Thoughts on the UI and functionality first -- mainly a number of style tweaks:
... you know what, I'll make this a thread on #mobile. That seems like the best venue for this part, because other people may have useful feedback too.

Comments on the UX are here:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/poll.20widget/near/768137
Reading the code now.
c174a0bd4 user selectors: Add selector for retrieving current user.

Cool, this looks helpful to add!

Let's avoid adding a use of NULL_USER; see its jsdoc :wink: . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)
In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently :slightly_smiling_face:

509d8e0a4 widget poll: Add basic support to show widgets from available data.

I like the idea to move widgetAsHtml to a new file!

nit: widget: Message | Outbox -- I'd call this just message. I feel like "widget" should refer to something more specific -- i.e. it's not that the message is a widget, but rather has a widget.

Here:

  // This condition will never practically happen as it has been confirmed in caller
  // -- just there for Flow's happiness
  if (!widget.submessages) {
    return '';
  }

When a condition will actually never happen, I'd rather not have code that only executes if it happens :slightly_smiling_face: . For one thing -- is returning '' really a good way to handle it if it somehow did?
One solution is a $FlowFixMe on a specific line that Flow complains at.
Here, I think a good solution is to listen to the type error as a signal that your interfaces don't match your logic -- you're in a condition where submessages is nonempty, but you're passing the whole message which is mostly not about that. How about passing submessages itself? And content.

Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

Important! Please use the HTML-escaping template template-tag for all HTML.
Without that, this has an HTML injection vulnerability where it inserts option.option. To illustrate, here's some options added by a different user, as seen in the webapp:

and then how that renders under the PR's current version:

Let's also replace the pieces.push pattern with template-tagged template literals. That makes fewer templates, making it easier to consistently use the tag; and it also makes the nesting of <div>, </div>, etc., easier to see.

I think a helpful simplification would be to do submessages.map(submessage => JSON.parse(submessage.content)) up front.

It's kind of confusing how pollOptions is built:

The initialization with a filter/map sequence, very functional-programming-style, makes it look like that should be the final, never-mutated data structure.
And indeed if I look for references to pollOptions, they all look like reads -- find, then forEach to build HTML.
But the pollVotes stuff must be doing something... and that variable doesn't appear in the HTML-building, hmm... aha, the pollOptions.find is secretly a setup for mutating the option's data!
A small change that would help: say const pollOptions = [], so it obviously gets mutated later. Then have a forEach instead of map.

I think the other uses of filter and map would also be improved by replacing with straightforward loop logic. Also we can then have just one loop, with a conditional on type, processing all the submessages in order. That's the canonical order, after all -- because that's what we'd get if we processed them incrementally as they happened.

Another tool that would simplify some of this code: use Map for pollOptions, instead of an array. Then in particular
pollOptions.find(option => option.id.toString() === votedOption)

gets a lot more straightforward.
(The current PR version is similar to lots of our existing code -- there's a lot of places Map would help.)

What's the corresponding webapp code? It'd be helpful to point to it here, to aid comparisons.

Naming: Let's s/title/question/g . That's what it is... and I think the border-bottom in this CSS (discussed in chat, linked above) is the sort of thing that would make more sense if it really were a "title" instead of a "question", so perhaps an example of this name leading one's thinking astray. :slightly_smiling_face:

be9d88444 submessages: Add vote ability to poll widgets.
e29641b88 widgets: Convert .widget class to a superclass.
(I'll leave these two here for today; the above, plus the UX comments in chat linked above, should be enough for the first round.)

(so I wrote this entire reply, and then it got erased because the browser closed, so the replies below will be terse in order to be efficient)

Let's avoid adding a use of NULL_USER; see its jsdoc 😉 . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)
In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently 🙂

Thanks! I saw those commits initially, but was reluctant to reject NULL_USER's use because the alternatives in my mind were very complicated to understand. Fixed now.

i.e. it's not that the message is a widget, but rather has a widget.

Makes sense. Fixed!

Here, I think a good solution is to listen to the type error as a signal that your interfaces don't match your logic -- you're in a condition where submessages is nonempty, but you're passing the whole message which is mostly not about that. How about passing submessages itself? And content.

I agree, thanks! Fixed!

Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

Sure, done!

Important! Please use the HTML-escaping template template-tag for all HTML

My bad, I assumed that option.option was markdown (as I mistakenly pointed out in the comment containing its type.
/**
 * Option submessages are of type -
 * {
 *    type: 'new_option',
 *    idx: number,
 *    option: string (Question String),
 * }
 */

A small change that would help: say const pollOptions = [], so it obviously gets mutated later. Then have a forEach instead of map.

Sure! That would help out a lot.

Let's also replace the pieces.push pattern with template-tagged template literals. That makes fewer templates, making it easier to consistently use the tag; and it also makes the nesting of <div>, </div>, etc., easier to see.

Well, I tried to make it as template tagged as possible, but couldn't achieve it completely because we need to use a for loop to cover all the options anyways. Do you perhaps mean that there need to be functions which return perhaps a Map of option (html) objects which can then be joined?

Another tool that would simplify some of this code: use Map for pollOptions,

Is there any particular reason why you're recommending Map? Is it somehow faster performance wise? -- ah, you're saying it'll be more simple. Done!

Naming: Let's s/title/question/g . That's what it is... and I think the border-bottom in this CSS (discussed in chat, linked above) is the sort of thing that would make more sense

Done! 😄
(I constructed some automation for rebasing PRs since code review usually involves a lot of rebasing and committing changed to a previous commit, which takes up a lot of time. That took a lot of time this round. Hopefully next round should be much swifter than normal -- noticed a saving of about 20-30 seconds in each of the last three changes I committed to this PR.)

(the failure in CI doesn't seem to be related to this PR.)

Thanks @ishammahajan for these revisions!

(so I wrote this entire reply, and then it got erased because the browser closed, so the replies below will be terse in order to be efficient)

Ouch, yeah. That is a frustrating limitation in GitHub. :cry: The same thing happens to me sometimes and makes me tempted to write my comments in my own editor first... but never quite often enough to get me to pay the overhead of actually doing that.
Hmm, reflecting on it, I think this is one of the reasons I've shifted to more often posting several separate comments if I have a lot to say. It limits just how much work I can lose if that happens.

Let's avoid adding a use of NULL_USER; see its jsdoc wink . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)
In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently slightly_smiling_face

Thanks! I saw those commits initially, but was reluctant to reject NULL_USER's use because the alternatives in my mind were very complicated to understand. Fixed now.

Looks great, thanks!
I think if I understand you right, the alternatives to NULL_USER (at least this alternative) are no longer very complicated in your mind. But if not, I'd be glad to explain more and/or answer followup questions :slightly_smiling_face:
About to merge that commit. I've also added a few new commits on top, mostly building on this in ways that came to mind when I was looking at it:

One delivers on what I said above about updating that comment
One adds jsdoc on this and some related selectors (I've added jsdoc on a lot of this file since you sent this PR)
One adds another selector getOwnUserId which this made easy to add -- @jainkuniya, this turns out to relate to something we were just talking about today :smile:, so take a look
One adjusts the use of caching

Next, I'll take a look at the rest of the branch.

About to merge that commit. I've also added a few new commits on top

(Done, as 3c25ad17f..75b8a3f84 )

931df06fb widget poll: Add basic support to show widgets from available data.

Style nit, in commit message: c174a0 is too short as a short-id for a commit. Very short short-ids risk becoming ambiguous as the history grows. Also GitHub's web UI won't linkify them: c174a0
I always write 9 digits, to make it very unlikely to ever become ambiguous. Saying git config --global core.abbrev 9 will make Git print that many whenever it prints a short-id, which helps.
By default Git abbreviates to 7 digits instead; so does GitHub; and that's also the minimum for GitHub's web UI to linkify it. So that's the minimum.

Important! Please use the HTML-escaping template template-tag for all HTML

My bad, I assumed that option.option was markdown (as I mistakenly pointed out in the comment containing its type.

I think you're correct about that! That's in fact key to why this is a problem. :stuck_out_tongue: I haven't really explained this very clearly, and it's important (in programming in general, and in this particular code), so let me try again.
First: though I haven't re-reproed it to confirm, I believe the same bug I demonstrated above is still present in your new version. The added use of template is exactly right, but then the $!$ re-introduces the bug. Would you try the repro in my screenshots above, and play around with that? I think that'd be a helpful hands-on exercise toward understanding this.
Then: to say more explicitly what I meant to say above, all code that produces HTML should use template.

By "produces HTML", I don't mean anything about the output's possible values -- like whether they might contain something like <strong>, or <script>, as a substring.
By "produces HTML", I mean that whatever consumes the output (the function's caller, or whatever) treats it as HTML. The central example of how to "treat it as HTML" is to stick it straight into the HTML fed to the WebView, without further transformation. In general the output of all our ...AsHtml functions will be treated as HTML -- we stick them into bigger and bigger pieces of HTML until the whole thing finally goes to the WebView.

Logically, "HTML" here is essentially a type. It's a subtype of string, with a meaning like "string that is OK to treat as HTML and won't break something if we do". We unfortunately don't have the type-checker helping us keep straight how we're handling this type, but the ways to reason about it are essentially the same as for any other type.
In particular, think of all the ...AsHtml functions as returning type "HTML", not the broader type string. Then when working inside of one, ask yourself: how do I know the value I'm returning is really a proper "HTML" ("string we can use as HTML without it being a bug") and not just any old string?

You can get a real "HTML" type by calling any other ...AsHtml function, because that's what they return.
Or with a string literal like '<div />', where you can look right at the source code and say "yep, that definitely is OK to use as HTML".
Or... by calling template. That's its job.

In particular, inside of template if you say ${...}, that takes a plain old string as argument... and it encodes it into HTML. This is the single primitive for turning plain strings into HTML -- any other function with a type like (string) => HTML ultimately uses template to do it (unless it contains a type error.) This lets us centralize in one place (the template function) the responsibility for doing that correctly.
There's also $!${...}. But: in that case you should think of the parameter type as "HTML". It's a type error (just one that Flow sadly doesn't help us catch) to pass an ordinary string to $!${...}.

So: without template, there was a type error because you were pushing into pieces -- which is an Array<HTML> -- a plain string returned by plain "``".
And then in this version, the template fixed that type error; but now you're passing option.option to $!${...}, which requires type HTML... and as you say, option.option is not type HTML. (It's some arbitrary text written by potentially any other user.) So that's a type error.
Does that help? I'd be glad to answer followup questions -- there are other aspects I could describe, but this is looking long already so I want to check in and gauge what would be helpful for you.

One helpful workaround strategy for the fact that the type-checker doesn't help us with this distinction is what we've done with all those ...AsHtml functions: when something has type HTML, put ...Html at the end of its name. This can make it much easier to look locally at any given bit of code and either confirm it's well-typed, or notice someplace that's potentially wrong -- where it can only be well-typed if some name isn't observing that convention.
I'd recommend doing that with the local names here, too: contentHtml, pollWidgetAsHtml, htmlPieces.

... I have another remark to make, but I'll hit send at this point first :sweat_smile:

931df06fb widget poll: Add basic support to show widgets from available data.
(cont'd)

Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

Sure, done!

Thanks!
I think I didn't succeed in explaining what I had in mind the first time, though. The most important part here is actually that the voters bit gets moved into that separate function too -- i.e. the pollVotes.forEach loop, which goes and constructs the voters property on each value in pollOptions.
That's partly because one of the really confusing things in this logic is the interaction between the code that computes pollOptions, and the code after that that goes back and mutates it. When that interaction is contained within a smaller function, it has a smaller effect on understanding the code.
It's also partly because the natural separation I mentioned is between pulling the data out of submessages -- i.e., computing pollOptions, in its final form, including the completed voters properties -- and then using that data to make HTML. Moving just one phase of the computation into its own function separates the logic in a way that's not as helpful for understanding it.

I think if I understand you right, the alternatives to NULL_USER (at least this alternative) are no longer very complicated in your mind. But if not, I'd be glad to explain more and/or answer followup questions 🙂

That's true! I'm pretty sure I understand it (at least enough that if I refer to this PR again, I will be able to solve further problems). Thanks!

One adds another selector getOwnUserId which this made easy to add -- @jainkuniya, this turns out to relate to something we were just talking about today 😄, so take a look

That sounds good!

Style nit, in commit message: c174a0 is too short as a short-id for a commit. Very short short-ids risk becoming ambiguous as the history grows. Also GitHub's web UI won't linkify them: c174a0

I'll fix that. Thanks!

By "produces HTML", I mean that whatever consumes the output (the function's caller, or whatever) treats it as HTML. The central example of how to "treat it as HTML" is to stick it straight into the HTML fed to the WebView, without further transformation.

Oh that makes it crystal clear!

Does that help?

Definitely! Thanks so much for the long explanation! 😄

I'd recommend doing that with the local names here, too: contentHtml, pollWidgetAsHtml, htmlPieces.

Makes a lot more sense, fixing. :)

... I have another remark to make, but I'll hit send at this point first 😅

Which reminds me, I should probably do the same.

@gnprice Thanks for the review! :)

That's partly because one of the really confusing things in this logic is the interaction between the code that computes pollOptions, and the code after that that goes back and mutates it.

Ah, that makes so much more sense, again. It's fixed in my WD now.

Moving just one phase of the computation into its own function separates the logic in a way that's not as helpful for understanding it.

Yeah, I don't remember why I thought that would even be a good idea now.

Thanks @ishammahajan for the revisions -- this looks much nicer! :smile:
d616773a4 widget poll: Add basic support to show widgets from available data.
Just a few comments at this stage -- and none of them critical like the HTML injection was. :wink:
  const pollOptions = submessages
    .filter(submessage => JSON.parse(submessage.content).type === 'new_option')
    .reduce((options, option) => {
      const rawOption = JSON.parse(option.content);
      const optionId = rawOption.idx;
      options.set(optionId.toString(), {
        id: optionId,
        option: rawOption.option,
        voters: [],
      });
      return options;
    }, new Map());

This is another example of the "false reduce" which we used to have all over, but which I recently purged from our codebase. ("Another" because I also mentioned this pattern just now on another PR: https://github.com/zulip/zulip-mobile/pull/3535#issuecomment-524120559 .) Notice how the reducer takes options in... but then always returns the same options, and does its work by mutating it. That's the opposite of what reduce is intended for, making it confusing to read.
A direct translation would be to use forEach, with an explicit accumulator variable:

  const pollOptions = new Map();
  submessages
    .filter(submessage => JSON.parse(submessage.content).type === 'new_option')
    .forEach(option => {
      const rawOption = JSON.parse(option.content);
      const optionId = rawOption.idx;
      pollOptions.set(optionId.toString(), {
        id: optionId,
        option: rawOption.option,
        voters: [],
      });
    });

I think a further improvement would be to fuse the two loops:
  const pollOptions = new Map();
  submessages.forEach(submessage => {
    const data = JSON.parse(submessage.content);
    if (data.type !== 'new_option') {
      return;
    }
    const optionId = data.idx;
    pollOptions.set(optionId.toString(), {
      id: optionId,
      option: data.option,
      voters: [],
    });
  });

One of my previous comments is I think still applicable:

I think a helpful simplification would be to do submessages.map(submessage => JSON.parse(submessage.content)) up front.

In particular note the only thing we ever do with submessages is pull off content from an element, and then hand that to JSON.parse. So the result of that map could replace everywhere we use submessages.
And on the other hand the current version can end up parsing a given submessage's content quite a few times.

Another one:

I think the other uses of filter and map would also be improved by replacing with straightforward loop logic. Also we can then have just one loop, with a conditional on type, processing all the submessages in order. That's the canonical order, after all -- because that's what we'd get if we processed them incrementally as they happened.

This is a lot like the changes I illustrate above for removing the false reduce. Perhaps it's easier to understand what I had in mind here with those examples? I think the refactoring you've done to make extractPollOptions might also help.
Again, though, this one isn't critical. If it's still unclear, the easiest way to explain may be for me to illustrate by merging, and then adding a commit or two on top to make these refactors :slightly_smiling_face:

Similarly I'd still like to replace the htmlPieces.push / join pattern with just template literals -- but I will probably just attempt that as a refactor on top after I merge this, and then that'll be an illustration to help explain what I mean.

  let pollOptions = new Map();

This is a dead store; just make it const later when it's assigned.

Quick thoughts outside of src/webview/html/:
@@ -69,6 +70,7 @@ export type BackgroundData = $ReadOnly<{
   flags: FlagsState,
   mute: MuteState,
   ownEmail: string,
+  ownUser: User,

A nice followup would be to drop ownEmail in favor of ownUser.email.

I haven't yet read the CSS closely.

eb6235875 submessages: Add vote ability to poll widgets.
+  if (target.matches('.poll-button')) {
+    const optionElement = target.closest('.poll-option-container');
+    if (!optionElement) {
+      throw new Error('Corresponding option element not found.');
+    }

Can we stick the data-option-id on the .poll-button instead? That'd simplify this a bit.
(If there's some reason that would make something else complicated, though, then no need.)

When you rebase, note c4fdd6584; the api usage has gotten a bit cleaner still, and you'll want to update to match.

            key: [ownUser.user_id, event.optionId].join(),

Huh, this... inserts a comma, I guess? TIL.
I'd rather be explicit. :slightly_smiling_face: Writing

`${ownUser.user_id},${event.optionId}`

isn't any longer, and I think clearer about the format of the string.
+        sendSubmessage(
+          auth,
+          event.messageId,
+          JSON.stringify({
+            type: 'vote',
+            key: [ownUser.user_id, event.optionId].join(),
+            vote: event.vote,
+          }),
+        );

Hmm, what source were you able to consult for seeing what the form of this request should be? Specifically what should go into the last argument, the JSON blob content. I don't see any details about votes on poll widgets in the widgets doc you helpfully linked to in the API-binding jsdoc.
Even if it's an unsatisfying answer like "the webapp code" or "the network tab of browser devtools using the webapp", that's useful context. And if it was source code, you can mention the particular file to look at.

(OK, stopping here.)

(I just edited the PR description to add a cross-reference to the issue, #3205.
That was already in one of the commits in the branch, which is important because the commits become our primary record of what we did... but because GitHub :man_shrugging: doesn't use that information as well as it could, it's separately important to mention any fixed issues in the PR description. In particular, this causes the issue thread to have a useful link back here to the PR.)

(The issue is P1, so this is too.)

What is the state on this pr?
I would really like to have this feature.
Can I help somehow?


Add debug log when JSON.parse blows up

Right now, the trickiest part of this lib is when you're dealing with a backend that does not respect the Accept header, and returns invalid JSON for certain endpoints. A good example are enterprise APIs that return stack traces for errors.
One way to help developers understand what's going on would be a log in development mode.
I'm thinking something like the following (this is kinda pseudocode):
let readBody;
if (process.NODE_ENV === 'development' && responseType === 'json') {
  try {
    readBody = body[responseType]();
  } catch(e) {
    console.log('Blah blah helpful text. Check out da docs 4 help.');
    throw e;
  }
} else {
  readBody = body[responseType]();
}

I don't think I'm going to be able to do this @marlonpp ☹ . The reading of the body happens in fetch-dedupe, which isn't a React lib, so I can't rely on the process.env.NODE_ENV trick...
I'm going to close this for now, but if anyone has an idea of how to do this without adding bloat to the final build, I am :+1: to hearing it. It should go on the fetch-dedupe project, though.  See https://github.com/jmeas/fetch-dedupe/issues/24 for more


WebGPU crash

I think Martin is aware of this, but adding here for tracking.
WebGPU examples crash on Win11 Chrome with error:

Hi .. thanks, I'm not aware. By the way, for completeness, which Chrome version are you using.

Version 121.0.6167.85 (Official Build) beta (64-bit)
Can't find it immediately, but we discussed it briefly in another topic. You said my browser reports something is supported, but not implemented and I that I should try to comment out something in the sources. Commenting it out removed the error.

For me on Win10:
Works: Version 120.0.6099.225 (Official Build) (64-bit)
Does not work: Version 121.0.6167.86 (Official Build) (64-bit)


on iOS, add(track) resolves to null

Describe the Bug
on iOS, the add method on TrackPlayer resolves to null
Steps To Reproduce
git clone git@github.com:DoubleSymmetry/react-native-track-player.git
cd react-native-track-player/example
yarn install
cd ios
pod install
cd ..
update ./App.tsx
diff --git a/example/App.tsx b/example/App.tsx
index b2e9c9d..3c4bb43 100644
--- a/example/App.tsx
+++ b/example/App.tsx
@@ -44,14 +44,16 @@ const setupIfNecessary = async () => {
     compactCapabilities: [Capability.Play, Capability.Pause],
   });

-  await TrackPlayer.add(playlistData);
-  await TrackPlayer.add({
+  const i = await TrackPlayer.add(playlistData);
+  console.log("added list:", i);
+  const ii = await TrackPlayer.add({
     url: localTrack,
     title: 'Pure (Demo)',
     artist: 'David Chavez',
     artwork: 'https://i.scdn.co/image/e5c7b168be89098eb686e02152aaee9d3a24e5b6',
     duration: 28,
   });
+  console.log("added single:", ii);

   TrackPlayer.setRepeatMode(RepeatMode.Queue);
 };

Environment Info:
react-native-track-player: 2.1.2

System:
    OS: macOS 11.6.2
    CPU: (8) x64 Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
    Memory: 1.22 GB / 16.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 16.11.1 - /usr/local/bin/node
    Yarn: 1.7.0 - /usr/local/bin/yarn
    npm: 8.0.0 - /usr/local/bin/npm
    Watchman: Not Found
  Managers:
    CocoaPods: 1.11.2 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 21.2, iOS 15.2, macOS 12.1, tvOS 15.2, watchOS 8.3
    Android SDK: Not Found
  IDEs:
    Android Studio: Not Found
    Xcode: 13.2.1/13C100 - /usr/bin/xcodebuild
  Languages:
    Java: 10.0.2 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2
    react-native: 0.65.1 => 0.65.1
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

How I can Help
Probably should update the documentation or update iOS. I sent in a pull request

The Documentation says it returns a number
Readme.md says it returns undefined
The iOS code itself resolves null
The Windows code also resolves null
For android

The MusicModule uses the MusicBinder handle the callback
Which provides a playback to do the actual add
But seems to provide the actual index

Temporary Solution
async function add(track, index){
  if(Platform.OS !== "ios") return TrackPlayer.add(track, index);
  if(typeof index === "number"){
    await TrackPlayer.add(track, index);
    return index;
  }
  const queue = TrackPlayer.getQueue();
  await TrackPlayer.add(track);
  return queue.length;
}

Nicely documented Issue. Thank you

Pull request merged : )


Remove from gitconfig

[filter "media"]
  clean = git-media-clean %f
  smudge = git-media-smudge %f

[filter "hawser"]
  clean = git hawser clean %f
  smudge = git hawser smudge %f
  required = true

[difftool "sourcetree"]
  cmd = opendiff \"$LOCAL\" \"$REMOTE\"
  path =
[mergetool "sourcetree"]
  cmd = /Applications/SourceTree.app/Contents/Resources/opendiff-w.sh \"$LOCAL\" \"$REMOTE\" -ancestor \"$BASE\" -merge \"$MERGED\"
  trustExitCode = true

Also, what's all that URL stuff? Might want to clean that up as well.

Done via a3f344d9c39db3fc118af6baa71c9404e195ec72


[multistage] Do Not Visit ValueNode's Input in Visitor

TODO: Add a UT
cc: @walterddr

Codecov Report

Merging #10163 (7a22871) into master (4d7da0a) will decrease coverage by 21.20%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #10163       +/-   ##
=============================================
- Coverage     34.84%   13.64%   -21.20%
+ Complexity      200      176       -24
=============================================
  Files          2006     1951       -55
  Lines        108718   106259     -2459
  Branches      16501    16208      -293
=============================================
- Hits          37880    14503    -23377
- Misses        67566    90614    +23048
+ Partials       3272     1142     -2130

Flag
Coverage Δ

integration1
?

integration2
?

unittests2
13.64% <ø> (+<0.01%)
:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...lanner/stage/DefaultPostOrderTraversalVisitor.java
0.00% <ø> (ø)

...src/main/java/org/apache/pinot/sql/FilterKind.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...ain/java/org/apache/pinot/core/data/table/Key.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...ain/java/org/apache/pinot/common/CustomObject.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...n/java/org/apache/pinot/core/data/table/Table.java
0.00% <0.00%> (-100.00%)
:arrow_down:

.../java/org/apache/pinot/core/data/table/Record.java
0.00% <0.00%> (-100.00%)
:arrow_down:

.../java/org/apache/pinot/core/util/GroupByUtils.java
0.00% <0.00%> (-100.00%)
:arrow_down:

.../java/org/apache/pinot/common/auth/AuthConfig.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...va/org/apache/pinot/core/routing/RoutingTable.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...ava/org/apache/pinot/sql/parsers/PinotSqlType.java
0.00% <0.00%> (-100.00%)
:arrow_down:

... and 749 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more


Closing dropdown with ESC key propagates and for example closes a Bootstrap Modal

Here is a fiddle to reproduce the issue with the latest code:
http://jsfiddle.net/9p9fsmfr/1/
When I open the dropdown, and then press ESC key, the dropdown closes, and so does the modal. I think the ESC key should be caught and prevented when the dropdown is open.

We've run into a similar problem with our custom modal implementation. You need to close the modal only if:

The key's an escape key, and
The event's default has not been prevented

The event is only having its default prevented but propagation isn't being stopped.

The ticket where we are tracking this issue for 4.0 is at https://github.com/select2/select2/issues/4218.

How come this issue is closed while it still exists??
#4218 is related to totally different situation.

Probably the problem is here:
https://github.com/twbs/bootstrap/blob/v4-dev/js/src/modal.js, line 298
Instead of
if (event.which === ESCAPE_KEYCODE)
use
if (event.which === ESCAPE_KEYCODE && !event.defaultPrevented)

@greendimka if you believe that this is still an issue, please create a working example that demonstrates the problem in the latest version (4.0.6-rc.1).  See our contributing guidelines.
This issue is from 2014, so @cervengoc's example is likely highly outdated.  I certainly can't seem to reproduce it in my code at all.

232323232323


Improve the filtering experience: add new operators and change some behaviors

The filtering is powerful and wildly feature of Meilisearch. By gathering some user feedback, we want to improve this experience.
This central issue gathers the different issues opened about the filtering

https://github.com/meilisearch/meilisearch/issues/2486
https://github.com/meilisearch/meilisearch/issues/2479
https://github.com/meilisearch/meilisearch/issues/2484
https://github.com/meilisearch/meilisearch/issues/2516

Here are the related product discussions

https://github.com/meilisearch/product/discussions/479
https://github.com/meilisearch/product/discussions/274
https://github.com/meilisearch/product/discussions/407

Process
💫 This issue will be completed with links and all the necessary information all along the feature creation process

[X] Create a prototype by providing a docker image

A docker tag is available:
docker run -it --rm \
    -p 7700:7700 \
    getmeili/meilisearch:v0.29.0-filter.beta.0

Or you can compile the source code on the filter/field-in branch on this repo
All the information about the new addition is detailed here

[ ] Apply changes according to feedback
[X] Create the corresponding specification and link it to this issue -> https://github.com/meilisearch/specifications/pull/163

⚠ if the discovery phase (prototype creation + user feedback) is not satisfying, we will postpone this feature addition

Hey @curquiza!  Thanks for the ping :) How can we help during the discovery phase ? We are currently working on finding a process to create releases with specific features that we want to let users try during the discovery phase.
Would this feature be something you would like to see implemented in a SDK and communicated to users so that they can try it more simply?

Note that after these changes are merged, the filter depth limitation will be eliminated, and thus the documentation will need to be updated :)  @curquiza

Is the limitation completely removed? Aren't the parenthesis ([]) increasing the filter depth?

Good point! I had to double-check.

if the filters are given as arrays instead of strings, the limitations are also removed.
but there remains a potential stack overflow for the filter: (((((...(((((((x = 1)))))))...))))). The stack overflow is not caught at the moment though, so it always crashes meilisearch instead of returning an error.

FYI, the corresponding spec has been opened by @loiclec 🎉  -> https://github.com/meilisearch/specifications/pull/163

Better late than never, a prototype is available, see how to use this issue description

If I use the docker for pagination like getmeili/meilisearch:v0.29.0-pagination.beta.3, does it also contain this filtering prototype. Or I need to use 2 different dockers to experiment?

Hello @mech
Sorry, only this docker image put in the issue description work for filter: getmeili/meilisearch:v0.29.0-filter.beta.0
However, the first RC of v0.29.0 is out next Monday, you will be able to test the full v0.29.0 release. But it does not contain the pagination feature we delayed due to a lack of feedback on it; we want to be sure before releasing it.

Hello everyone watching this issue!
We have just released v0.29.0rc1, which is a release candidate of v0.29.0 🔥
It means you can test the just-introduced EXISTS and IN filtering operators! Binaries are attached to the release, or you can you the docker image:
docker run -it --rm \
    -p 7700:7700 \
    getmeili/meilisearch:v0.29.0rc0

Let us know about any bugs or feedback! 😄 It would be really helpful.
FYI, the official v0.29.0 release will be available on 3rd October.

If I am not wrong, this RC1 release is only having the filtering prototype right, without the pagination prototype.


Inferred frequency does not conform In Calculating TEARS

Here's the thing, I was dealing with a problem caused by pandas.datetimeindex.frequency
The api returned clean factor successfully, and I checked the .freq attribute, it seems in accordance with my input factor data:

when I ran:

alphalens.tears.create_returns_tear_sheet(factor_data, long_short=True, group_neutral=True, by_group=False)

I got the error:
F:\Anaconda3\lib\site-packages\alphalens-0.3.0-py3.6.egg\alphalens\tears.py in create_returns_tear_sheet(factor_data, long_short, group_neutral, by_group)
    316     factor_returns = perf.factor_returns(factor_data,
    317                                          long_short,
--> 318                                          group_neutral)
    319
    320     mean_quant_ret, std_quantile = \

F:\Anaconda3\lib\site-packages\alphalens-0.3.0-py3.6.egg\alphalens\performance.py in factor_returns(factor_data, demeaned, group_adjust, equal_weight, by_asset)
    243
    244     weights = \
--> 245         factor_weights(factor_data, demeaned, group_adjust, equal_weight)
    246
    247     weighted_returns = \

F:\Anaconda3\lib\site-packages\alphalens-0.3.0-py3.6.egg\alphalens\performance.py in factor_weights(factor_data, demeaned, group_adjust, equal_weight)
    203
    204     # preserve freq, which contains trading calendar information
--> 205     weights.index.levels[0].freq = factor_data.index.levels[0].freq
    206     return weights
    207

F:\Anaconda3\lib\site-packages\pandas\core\indexes\datetimelike.py in freq(self, value)
    239         if value is not None:
    240             value = frequencies.to_offset(value)
--> 241             self._validate_frequency(self, value)
    242
    243         self._freq = value

F:\Anaconda3\lib\site-packages\pandas\core\indexes\datetimelike.py in _validate_frequency(cls, index, freq, **kwargs)
    228             msg = ('Inferred frequency {infer} from passed values does not '
    229                    'conform to passed frequency {passed}')
--> 230             raise ValueError(msg.format(infer=inferred, passed=freq.freqstr))
    231
    232     @property

ValueError: Inferred frequency None from passed values does not conform to passed frequency C

Seems the problem was caused by internal attribute weights. Any way I can fix this?

Thank you for reporting this @hexiaoling1994.

Could you provide the details of what numpy and pandas versions you are using?
Does this error happen all the time or it is restricted to certain input data only?
if you run Alphalens example NBs (the ones with synthetic data) do they work properly or do you experience the same issue reported here?

@hexiaoling1994 Could you also make sure you are using the latest Alphalens version 3.2?
>>> import alphalens
>>> alphalens.__version__
'0.3.2'

I checked the versions and updated numpy, pandas, alphalens to the latest.
And I tried:
factor_data = alphalens.utils.get_clean_factor_and_forward_returns(
            factor, prices, groupby, by_group=False,
            quantiles=10, bins=None, periods=(2,5,8))

Another error happened, which also seems highly related to frequency of index:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-31-68a918e79fe3> in <module>()
      1 factor_data = alphalens.utils.get_clean_factor_and_forward_returns(
      2             factor, prices, groupby, by_group=False,
----> 3             quantiles=10, bins=None, periods=(2,5,8))

F:\Anaconda3\lib\site-packages\alphalens\utils.py in call_w_context(*args, **kwargs)
    609                           "replaced by 'binning_by_group'",
    610                           category=DeprecationWarning, stacklevel=3)
--> 611         return func(*args, **kwargs)
    612     return call_w_context
    613

F:\Anaconda3\lib\site-packages\alphalens\utils.py in get_clean_factor_and_forward_returns(factor, prices, groupby, binning_by_group, quantiles, bins, periods, filter_zscore, groupby_labels, max_loss)
    762
    763     forward_returns = compute_forward_returns(factor, prices, periods,
--> 764                                               filter_zscore)
    765
    766     factor_data = get_clean_factor(factor, forward_returns, groupby=groupby,

F:\Anaconda3\lib\site-packages\alphalens\utils.py in compute_forward_returns(factor, prices, periods, filter_zscore)
    281             start = prices.index[p_idx]
    282             end = prices.index[p_idx + period]
--> 283             period_len = diff_custom_calendar_timedeltas(start, end, freq)
    284             days_diffs.append(period_len.components.days)
    285

F:\Anaconda3\lib\site-packages\alphalens\utils.py in diff_custom_calendar_timedeltas(start, end, freq)
    920     if weekmask is not None and holidays is not None:
    921         # we prefer this method as it is faster
--> 922         actual_days = np.busday_count(start, end, weekmask, holidays)
    923     else:
    924         # default, it is slow

TypeError: Iterator operand 0 dtype could not be cast from dtype('<M8[us]') to dtype('<M8[D]') according to the rule 'safe'

Here's how the index looks like, I can't tell which part of it was wrong...
>>>factor.index.levels[0]
DatetimeIndex(['2016-01-04', '2016-01-05', '2016-01-06', '2016-01-07',
               '2016-01-08', '2016-01-11', '2016-01-12', '2016-01-13',
               '2016-01-14', '2016-01-15',
               ...
               '2018-08-13', '2018-08-14', '2018-08-15', '2018-08-16',
               '2018-08-17', '2018-08-20', '2018-08-21', '2018-08-22',
               '2018-08-23', '2018-08-24'],
              dtype='datetime64[ns]', name='DATES', length=690, freq='B')

the other inputs also seemed ok...

I am closing this since no answer to my questions were provided. If you still have the issue feel free to re-open this but please answer those questions:

Could you provide the details of what numpy and pandas versions you are using?
Does this error happen all the time or it is restricted to certain input data only?
if you run Alphalens example NBs (the ones with synthetic data) do they work properly or do you experience the same issue reported here?

Hi,
I'm running into same exact issue. Getting it while running the simplest example notebook.
Pandas is 0.23.4, numpy is 1.15.3.
Cheers mate.

I discovered the problem is  Pandas version 0.23.4. In our tests we cover pandas 0.20.3 (see here. Until we make Alphalens compatible with more recent versions of Pandas please stick with version  0.20.3


Add event manager

Codecov Report
Base: 70.58% // Head: 58.33% // Decreases project coverage by -12.25% :warning:

Coverage data is based on head (cac809c) compared to base (e533171).
Patch coverage: 15.09% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #14       +/-   ##
===========================================
- Coverage   70.58%   58.33%   -12.26%
===========================================
  Files          15       18        +3
  Lines         187      240       +53
===========================================
+ Hits          132      140        +8
- Misses         55      100       +45

Impacted Files
Coverage Δ

src/events/EventManager.cpp
0.00% <0.00%> (ø)

src/events/EventManagerImpl.cpp
100.00% <100.00%> (ø)

src/events/Listener.h
100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Add deno fmt as an executor

Deno has a formatter that can be called via deno fmt. It would be great if there was an executor that would call this under the hood to keep consistency with the lint and other executors currently available.
Edit:
My current workaround is to add a manual fmt entry to my project.json. It looks something like this
"fmt": {
  "executor": "nx:run-commands",
  "options": {
    "command": "deno fmt --config apps/my-app/deno.json"
  }
}

Hi @adume, thank you for opening the issue. We initially thought about adding the fmt target but we felt like it's not worth it, especially when you're in an Nx workspace with Prettier and Deno Formatter combined. In addition, nx format uses prettier and will use Prettier configuration to format the whole workspace whereas deno fmt runs using its own configuration unless you match the prettier config with deno config.
At the moment, prettier can format Deno files well enough (as they are just TypeScript). If deno fmt is something you want to use for your project, then the workaround using run-commands would be sufficient enough. You can also use the short-hand syntax
"fmt": {
  "command": "deno fmt --config apps/my-app/deno.json"
}


Makefile: install headers with dynamic library

Currently, headers are only installed with static library
Signed-off-by: Fabrice Fontaine<EMAIL_ADDRESS>

Do you have some comments on this PR?

Header files are usually unnecessary when installing a dynamic library.
The use case for install-lib-shared target is to install or upgrade the librhash.so for usage by a binary program depending on it. This target is also used by Debian to build librhash0*.deb package, which must not contain headers.
For development purpose you should run make install-lib-shared install-lib-headers.
Note that the install-lib-static target installs header files for convenience, since a static library is used for development, which usually implies compilation of source code depending on the headers.

OK, thanks for your feeback, then I'm closing this PR.