Jingle and Jingle File Transfer #489

gkfabs · 2021-09-07T20:58:59Z

This PR adds initial support (elements and providers) for XEP-0166 and XEP-0234

I took the work from https://github.com/vanitasvitae/Smack/tree/jftPR and made the modifications to the new code base.

gkfabs · 2021-10-23T03:53:29Z

That would be great to have jingle file transfert to be able to send image files (camera, graph, etc...) to jabber client from openhab. What should I do to have a small review of the code quality ?

Thanks

Flowdalic · 2021-10-28T11:45:27Z

What should I do to have a small review of the code quality ?

Sorry for the late response. My priority is to get the next Smack release out, but after that, I'll look at your code. Meanwhile, I have tirggered a CI run for this PR. Feel free to address any issues potentially found by the run.

gkfabs · 2021-10-28T14:17:15Z

Thanks you so much! I'll address the issues from the CI in the next 7 days.

Flowdalic · 2021-10-28T15:25:44Z

Note that you can always run the checks locally via gradle check. I suggest doing so as part of your development cycle :)

gkfabs · 2021-10-28T17:34:22Z

On my local it is working without error

fabs@FabsLaptop:~/programme/Smack/smack-experimental (master)$ gradle check
Starting a Gradle Daemon (subsequent builds will be faster)

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.1.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 4m 0s
33 actionable tasks: 25 executed, 8 up-to-date

That must be a version problem of some kind. I'll find it.

Thanks

Flowdalic · 2021-10-28T17:47:10Z

On my local it is working without error
fabs@FabsLaptop:~/programme/Smack/smack-experimental (master)$ gradle check

Run gradle check in Smack's root, not in a Smack subproject directory.

gkfabs · 2021-10-28T23:07:30Z

yes sorry I went to quickly over it. That should be the right one

fabs@FabsLaptop:~/programme/Smack (master)$ gradle check

> Task :smack-extensions:test
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

> Task :smack-repl:scalaStyleMainCheck
Processed 1 file(s)
Found 0 warnings
Found 0 errors
Finished in 515 ms

> Task :smack-websocket:test
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

> Task :smack-websocket-okhttp:test
OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.1.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 7m 2s
309 actionable tasks: 258 executed, 51 up-to-date

Flowdalic

I've stopped the review half though, since this appears to be a half baked pull request (why isn't it marked WIP?). I think, if you want to continue working on Jingle support, then the initial work should be done outside of the Smack repository. Create an extra library with your code, that pulls in Smack as dependency. And, once the code is ready, it can be donated back to Smack.

Flowdalic · 2021-11-04T08:41:32Z

build.gradle

@@ -131,6 +131,9 @@ allprojects {
 		androidJavadocOffline = { getAndroidJavadocOffline() }
 		junit4Projects = [
 			':smack-core',
+			':smack-extensions',


I am trying to phase out junit 4 in favor of junit 5 in Smack, so please don't add more projects here.

Flowdalic · 2021-11-04T08:42:47Z

...tal/src/main/java/org/jivesoftware/smackx/jingle_filetransfer/JingleFileTransferManager.java


 /**
- * Manager for JingleFileTransfer (XEP-0234).
+ * Manager for XEP-0234 - JingleFileTransfers.


Not sure why this was changed. I liked the old style more, as XEP numbers are less important than humand readable text. But the missing spaces should be added, after all, the title of the XEP is "Jingle File Transfer".

Flowdalic · 2021-11-04T08:43:28Z

...in/java/org/jivesoftware/smackx/jingle_filetransfer/component/AbstractJingleFileRequest.java

+
+/**
+ * Class representing the base of a JingleFileRequest.
+ * TODO: Implement.


So this is not implemented? Why is it here than?

Flowdalic · 2021-11-04T08:44:38Z

...main/java/org/jivesoftware/smackx/jingle_filetransfer/component/JingleIncomingFileOffer.java

+                    break;
+                }
+            }
+            LOGGER.log(Level.FINE, "Reading/Writing finished.");


Those log statements are harmful. Especially when they only consist of a static string.

Flowdalic · 2021-11-04T08:45:09Z

...in/java/org/jivesoftware/smackx/jingle_filetransfer/component/JingleIncomingFileRequest.java

+
+/**
+ * Backend logic of an incoming file request (They request a file).
+ * TODO: Implement.


Again, this is not implemented? Then why isn't this PR marked as WIP?

gkfabs · 2021-11-16T19:19:07Z

Thanks for running again the CI tools against my PR.

The 'TODO: Implement.' comments you spot in the code are wrong. The class are definitively implemented. I'll do the correction. I'll changes also to junit 5, the other comment readability problem and the logging.

If I do an extra library with this code, can I still use the same package names as yours ? ie org.jivesoftware.smackx ?

Flowdalic · 2021-11-17T21:14:46Z

If I do an extra library with this code, can I still use the same package names as yours ? ie org.jivesoftware.smackx ?

I'd suggest that you substitute org.jivesoftware for a domain under your control (or, if you don't have one, something else). Then, if the code gets donated back, we switch the classes back into a package under org.jivesoftware, while keeping the trailing package names. This way, users can simply switch by performing a global search and replace in their codebase.

Flowdalic · 2023-03-17T16:56:50Z

Closing, feel free to reopen a new PR once you want to donate code back for review.

gkfabs mentioned this pull request Sep 13, 2021

[xmppclient] Add send image throught HTTP openhab/openhab-addons#11247

Merged

gkfabs force-pushed the master branch 7 times, most recently from c6918f1 to 0bb230e Compare October 23, 2021 03:52

gkfabs force-pushed the master branch 2 times, most recently from fb5152f to 58f7163 Compare October 27, 2021 17:25

gkfabs force-pushed the master branch from 58f7163 to 3ce304e Compare October 28, 2021 22:47

gkfabs added 2 commits October 29, 2021 12:29

Add Jingle changes to smack-extensions

6bfb12c

Add Jingle changes to smack-experimental

4d0846d

gkfabs force-pushed the master branch from 3ce304e to 4d0846d Compare October 29, 2021 17:29

Flowdalic requested changes Nov 16, 2021

View reviewed changes

Flowdalic closed this Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jingle and Jingle File Transfer #489

Jingle and Jingle File Transfer #489

gkfabs commented Sep 7, 2021

gkfabs commented Oct 23, 2021

Flowdalic commented Oct 28, 2021

gkfabs commented Oct 28, 2021

Flowdalic commented Oct 28, 2021 •

edited

Loading

gkfabs commented Oct 28, 2021

Flowdalic commented Oct 28, 2021

gkfabs commented Oct 28, 2021

Flowdalic left a comment

Flowdalic Nov 4, 2021

Flowdalic Nov 4, 2021

Flowdalic Nov 4, 2021

Flowdalic Nov 4, 2021

Flowdalic Nov 4, 2021

gkfabs commented Nov 16, 2021

Flowdalic commented Nov 17, 2021

Flowdalic commented Mar 17, 2023

Jingle and Jingle File Transfer #489

Jingle and Jingle File Transfer #489

Conversation

gkfabs commented Sep 7, 2021

gkfabs commented Oct 23, 2021

Flowdalic commented Oct 28, 2021

gkfabs commented Oct 28, 2021

Flowdalic commented Oct 28, 2021 • edited Loading

gkfabs commented Oct 28, 2021

Flowdalic commented Oct 28, 2021

gkfabs commented Oct 28, 2021

Flowdalic left a comment

Choose a reason for hiding this comment

Flowdalic Nov 4, 2021

Choose a reason for hiding this comment

Flowdalic Nov 4, 2021

Choose a reason for hiding this comment

Flowdalic Nov 4, 2021

Choose a reason for hiding this comment

Flowdalic Nov 4, 2021

Choose a reason for hiding this comment

Flowdalic Nov 4, 2021

Choose a reason for hiding this comment

gkfabs commented Nov 16, 2021

Flowdalic commented Nov 17, 2021

Flowdalic commented Mar 17, 2023

Flowdalic commented Oct 28, 2021 •

edited

Loading