Hey @st-rm-ng!

Thank you for contributing to JabRef! Your help is truly appreciated ❤️.

We have automatic checks in place, based on which you will soon get automated feedback if any of them are failing. We also use TragBot with custom rules that scans your changes and provides some preliminary comments, before a maintainer takes a look. TragBot is still learning, and may not always be accurate. In the "Files changed" tab, you can go through its comments and just click on "Resolve conversation" if you are sure that it is incorrect, or comment on the conversation if you are doubtful.

Please re-check our contribution guide in case of any other doubts related to our contribution workflow.

Hello @koppor and @InAnYan,

this is just a Draft PR, and it's not ready to be tested or anything. Think of this as my initial attempt or establishment to view all changes. I will let you know when it will be in ready for review state.

Thank you very much for your quick response to this PR 😄

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task.

Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task.
Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

For timeouts you could leverage the functionality of the Completable Future https://www.baeldung.com/java-completablefuture-timeout
I don't think rxjava would be needed here

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task.
Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

For timeouts you could leverage the functionality of the Completable Future https://www.baeldung.com/java-completablefuture-timeout
I don't think rxjava would be needed here

@st-rm-ng please follow that advice.

Okay, I only reviewed the code, but I find it very clear and easy to read. But may I ask this question: why there is at all some complicated logic for fetching the models? Why do you need to create a fetch thread and check that it's alive?

I think 1 BackgroundTask (which you already have) that is executed with a TaskExecutor on the start of JabRef is enough. And in case user wants to refresh, you can start that task when preferences window is open, or add a ⟳ button near the chat model field

Sorry, maybe I miss something, but I still don't get why implementing timeout is that hard and how it can block the background tasks? (I think we use a thread pool)

Here is an example how to send http request with timeout that ChatGPT generated me:

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.time.Duration;

public class SimpleHttp {
    public static void main(String[] args) throws Exception {
        HttpClient client = HttpClient.newBuilder()
                .connectTimeout(Duration.ofSeconds(5))  // connection timeout
                .build();

        HttpRequest request = HttpRequest.newBuilder()
                .uri(new URI("https://example.com"))
                .timeout(Duration.ofSeconds(3))        // request timeout
                .GET()
                .build();

        HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
        System.out.println(response.body());
    }
}

When it reaches timeout, an exception will be thrown.

BackgroundTask may process it by (if I recall correctly) task.onError(e -> {...})

Sorry, maybe I just don't know something about Java

@InAnYan Thank you for pointing out. Your code should solve the issue. @st-rm-ng Please incorporate that idea into your solution!

@koppor I've made modifications to use Nullmarked and Nullable as you hinted me. Is this what you had in your mind?

Currently no user-visible change, is it?\n\nI think, the model list in the preferences should be updated, shouldn't hey? Did I miss something?

@st-rm-ng what's the answer for that?

Currently no user-visible change, is it?\n\nI think, the model list in the preferences should be updated, shouldn't hey? Did I miss something?

@st-rm-ng what's the answer for that?

I haven't connected this back-end code to UI yet, still working on it

@koppor I've made modifications to use Nullmarked and Nullable as you hinted me. Is this what you had in your mind?

Nice one thank you. Looking forward for the update.

Although the UI is missing I think it would be good start to have this
@InAnYan what yoo you tink?

Although the UI is missing I think it would be good start to have this @InAnYan what yoo you tink?

I agree. We can add a follow-up issue.