Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add freerdp backend #42

Merged
merged 11 commits into from
Oct 9, 2023
Merged

Add freerdp backend #42

merged 11 commits into from
Oct 9, 2023

Conversation

LDprg
Copy link
Member

@LDprg LDprg commented Sep 10, 2023

This moved from #30 to here for better way of working.

@LDprg LDprg self-assigned this Sep 10, 2023
@LDprg LDprg added rewrite Using the winapps rewrite priority: high enhancement New feature or request labels Sep 10, 2023
winapps/src/freerdp.rs Outdated Show resolved Hide resolved
@LDprg
Copy link
Member Author

LDprg commented Oct 7, 2023

So I think generally the the base is finished and ready for merge. Just waiting for some reviews.

Maybe we want to change to command names (later this would we much more complicated).

Copy link
Member

@oskardotglobal oskardotglobal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@oskardotglobal
Copy link
Member

oskardotglobal commented Oct 7, 2023

Maybe we want to change to command names

We could just keep what you've called app as manual, but imo run would be better

@AkechiShiro
Copy link

@oskardotglobal did you try and build this branch ? Or should I test it on my side ?

@oskardotglobal
Copy link
Member

Nope, I'll test this later

@AkechiShiro
Copy link

AkechiShiro commented Oct 7, 2023

I tested the build works, but there are a few issues, I noticed @LDprg

@oskardotglobal
Copy link
Member

@AkechiShiro the latter is included in #26

@LDprg
Copy link
Member Author

LDprg commented Oct 8, 2023

@AkechiShiro The first one is that winapps doesnt disable the freerdp output (which contains alot of unnecessary warings). I will investigate into this.

@LDprg
Copy link
Member Author

LDprg commented Oct 8, 2023

@AkechiShiro did you test it with the generated quickemu from #26? I am not sure if we need a port forwarding from the guest to host to make rdp communication working per default.

@AkechiShiro
Copy link

@LDprg no I removed all the previous files to test cleanly but I understand that there is no quickemu get and VMs files

@oskardotglobal
Copy link
Member

@LDprg I'll test it

winapps-cli/src/main.rs Fixed Show fixed Hide fixed
@LDprg
Copy link
Member Author

LDprg commented Oct 9, 2023

So finally I fixed the log outputs of freerdp causing unessecary warnings to be shown. Later we could redirect its output to some log files, however this will be in a sperate PR (just want this get through as fast as possible).

Furthermore I fixed some minor issues after the merge of #26 and renamed the app command to run.
@oskardotglobal can you review this again so I can merge.

@LDprg LDprg requested a review from oskardotglobal October 9, 2023 08:33
Copy link
Member

@oskardotglobal oskardotglobal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise

winapps/src/freerdp.rs Outdated Show resolved Hide resolved
@LDprg LDprg requested a review from oskardotglobal October 9, 2023 13:25
@LDprg
Copy link
Member Author

LDprg commented Oct 9, 2023

@oskardotglobal I removed it.

Copy link
Member

@oskardotglobal oskardotglobal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@oskardotglobal oskardotglobal merged commit 3e8c6db into rewrite Oct 9, 2023
4 checks passed
@oskardotglobal oskardotglobal deleted the rewrite-freerdp-backend branch August 30, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: high rewrite Using the winapps rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants