Refactor AsyncTags

[mvaneijk]

Hi @extrawurst, I thought it would be a good idea to push my preliminary solution because I have some questions.

1. I looked at the other 2 implementations of AsyncSingleJob, do you think the solution is the right direction?
2. Why is there a 3 second timer in the current get-tag code? It seems interlinked with the async code, but it doesn't appear in the AsyncSingleJob struct. Is it still needed?
3. It seems there is a regression that removes the commit messages in the tags list, is that on purpose? does it need fixing? can I fix it directly in this PR? (introduced with 0f89693) - ah, it seems an issue with tui or crossterm (maybe broken API?)

Implementation

1. It took some effort, and it probably is very much obvious that my previous rust experience is close to 0, but I think I have build an (at least functional) refactor.
2. I wanted to preserve the "unchanged" notification functionality used in AsyncTags, so I moved it over to AsyncJob

Review

@extrawurst, can you perform a review and give me some guidance on what parts to improve?

Documentation

This Pull Request fixes/closes #757.

It changes the following:

• Refactor AsyncTags to AsyncTagsJob
• (Fix regression: missing commit messages in TagList?)

I followed the checklist:

• n/a - refactor
• I ran make check without errors
• I tested the overall application
• I added an appropriate item to the changelog

[extrawurst]

I don't think we need to adjust anything inside here (asyncjob/mod.rs) all of this is implementation specific to the asynctags side and therefore should be limited to it

[mvaneijk]

I agree, but how to be able to send two different notifications than? mod.rs only returns 1.

Although, an option might be to write a wrapper around tags.rs that does the hashing. I'll give that a try.

[extrawurst]

Hi @mvaneijk sorry for making you digg into this without having checked deeper myself. turns out the asyncjob abstraction was not good ending yet to allow you to do this in a nice way.

I pushed a few additions now:

• run now returns a notification value we want to be sent out when the job finished, this way you can customise depending on notify
• run takes a RunParams type as a param now which allows us to send intermediate updates out (this is needed to do stuff like the push/pull progress bar) (see in action here: syntax highlighting progress #889 (comment))

I think this should provide everything you need.
For the is_outdated and force stuff you can write a simple wrapper around AsyncSingleJob<YouTagsJob> that uses take_last, stores the last result and uses it to always be able to check whether the last result is outdated. I hope I make sense :D

[mvaneijk]

Hi @extrawurst, thanks for the effort. No worries, this is a good exercise for me to get acquainted with Rust.

Regarding the 'simple' wrapper. I will think of something. But I guess I probably end up creating another AsyncSingleJob for this wrapper/awaiter, because it can only compare the last_job hash after the tagsjob has finished. And the wrapper cannot be blocking while awaiting the result, hence should be an AsyncJob.

[mvaneijk]

hi @extrawurst, I don't know how to perform the hashcheck without changing the AsyncSingleJob code.

Situation

1. What is needed, is a function that
   1. performs the hash of the result upon completion of the get_tags function.
   2. compares the hash to the last result stored in the queue
   3. sends the appropriate notification according to the hash comparison
2. However, this function cannot be part of the AsyncJob, since the AsyncJob knows nothing about other AsyncJobs in the queue
3. This function cannot be part of a synchronous wrapper, because it has to wait on the AsyncJob result before running the hash function, and we don't want it to block.
4. This function cannot be part of an asynchronous wrapper (instantiating another AsyncSingleJob queue, because previous three points will recursively apply on the new instance.

I think there are 2 options

1. Make it possible to run a function at AsyncSingleJob scope upon completion of the AsyncJob
2. Make the last_result available for the current running AsyncJob.

Both need changing of the AsyncSingleJob code, what do you think?

~ edit: after some rethink, I guess option 1 makes the most sense. Probably a "send_notification" function as part of the AsyncJob trait that gets access to last_result. Sounds like a combination of the two :)

[mvaneijk]

The eagle has landed

I guess I've seen the light :)

@extrawurst, can you review it?

[extrawurst]

The eagle has landed

I guess I've seen the light :)

haha that sounds great! ❤️

@extrawurst, can you review it?

I will, as soon as I am back from my vacation!

[extrawurst]

I checked out the branch and played around in it and noticed the tags are not showing anymore in the log tab:

master:

[mvaneijk]

Good catch, I will look into it.

[extrawurst]

@mvaneijk are you still on this?

[mvaneijk]

I will try again today and let you know if I give up after that. I have a hard time debugging the issue,

[mvaneijk]

@extrawurst feel free to assign someone else to it. Maybe part or my PR can be reused, I feel that it is something small but I will probably have time again in the Christmas Holidays.

[extrawurst]

putting into draft until picked up again

[stale]

This issue has been automatically marked as stale because it has not had any activity half a year. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[mvaneijk]

This has been merged a while ago in 2022 =D, thx @guzzit