Skip to content

add protocol developer documentation#8666

Merged
nbolton merged 1 commit intodeskflow:masterfrom
laz-001:i8410-protocol-docs
Jun 25, 2025
Merged

add protocol developer documentation#8666
nbolton merged 1 commit intodeskflow:masterfrom
laz-001:i8410-protocol-docs

Conversation

@laz-001
Copy link
Copy Markdown
Contributor

@laz-001 laz-001 commented Jun 9, 2025

/claim #8410

Can be merged after:

First Part

The first part of this work (not merged) :

(EDIT: Lost my flow a bit, took this path to be able to go on. I am currently revising this locally within my own setup, which allows me to iterate fast, without involving a full build.)

@sithlord48
Copy link
Copy Markdown
Member

The new md files need to be added to the dev-docs sources so they should up in an IDE.
You also can update the protocoltypes.h and the mainpage .

@laz-001

This comment was marked as outdated.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from a83437c to 41243ef Compare June 10, 2025 18:00
@sithlord48
Copy link
Copy Markdown
Member

sithlord48 commented Jun 10, 2025

@laz-001 we have landed the dev doc starting parts so be sure you rebase upon our current master, Just make sure you rebase Don't Merge update that will cause landing issues later on.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 41243ef to 6541536 Compare June 10, 2025 21:39
@laz-001

This comment was marked as outdated.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 6541536 to 4bdbc53 Compare June 10, 2025 23:11
@sithlord48
Copy link
Copy Markdown
Member

I am going to say we don't need the tutorial and much of what is in the reference should perhaps be doxygen comments within protocolTypes or other files .. ill let @nbolton comment there as my main issues were related to dev document deployment.

@laz-001

This comment was marked as outdated.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 4bdbc53 to 104d55b Compare June 10, 2025 23:54
@sithlord48
Copy link
Copy Markdown
Member

@sithlord48 , i noticed the "about" section points to "deskflow.org", which points back to the repo. Maybe replace it with "https://deskflow.github.io/deskflow/"

@nbolton , you can review now.

I would not since the about should be user facing but we do need to link something to the dev docs

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 104d55b to 7ab80d1 Compare June 11, 2025 00:43
@laz-001

This comment was marked as outdated.

@sithlord48
Copy link
Copy Markdown
Member

You have removed my links from the main page to the other pages i.e building / contributing also you have made no links to the new pages how are the users expected to find the pages ?

Copy link
Copy Markdown
Member

@sithlord48 sithlord48 left a comment

Choose a reason for hiding this comment

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

Please put the links to the pages on the main page . Also put back the links to the build / contribute pages as well please

@laz-001

This comment was marked as resolved.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 7ab80d1 to 357599d Compare June 11, 2025 13:48
@sithlord48 sithlord48 requested a review from nbolton June 11, 2025 14:50
@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 357599d to 1d1f0cf Compare June 11, 2025 17:09
@sithlord48
Copy link
Copy Markdown
Member

The diagram is fine just put links to the other markdown files.

@laz-001

This comment was marked as outdated.

@laz-001
Copy link
Copy Markdown
Contributor Author

laz-001 commented Jun 13, 2025

@whot, could you take a look.
@nbolton , can we speed this up a bit? I'd like to finish this before going on with your other 2 main issues:

@sithlord48
Copy link
Copy Markdown
Member

You need to add a link to the protocol_reference.md in the main page

@laz-001

This comment was marked as outdated.

@sithlord48
Copy link
Copy Markdown
Member

No need for repetitions, I'm awaiting the main review so i can go on.

👋 I am part of that main review, While we wait for the others to lets get my simple nits out of the way.

@sithlord48 sithlord48 self-requested a review June 20, 2025 19:22
Copy link
Copy Markdown
Member

@sithlord48 sithlord48 left a comment

Choose a reason for hiding this comment

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

Ok lint fixed re approving, good from my side we will wait for Nick to approve and land it

@laz-001

This comment was marked as outdated.

@nbolton nbolton force-pushed the i8410-protocol-docs branch from 9588a49 to 70c5bac Compare June 23, 2025 10:34
Copy link
Copy Markdown
Member

@nbolton nbolton 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 apart from a couple of minor points. I probably missed some things as that's a lot of documentation to grok in one go, but I think we're almost good to land.

Copy link
Copy Markdown
Member

@sithlord48 sithlord48 left a comment

Choose a reason for hiding this comment

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

sorry i missed one thing.

@laz-001
Copy link
Copy Markdown
Contributor Author

laz-001 commented Jun 23, 2025

that's a lot of documentation to grok in one go

It's an iterative process. Personally, I see many things when the docs are public facing.

fixed those.

To speed things up: if you see more things, please feel free to directly commit.

sithlord48
sithlord48 previously approved these changes Jun 23, 2025
Copy link
Copy Markdown
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

Only managed to do a skim, completely flat out atm but this LGTM, thanks. I think it meets the requirements of "can someone implement the protocol based on the documentation here?". The proof will be in the pudding when someone actually tries it :)

@nbolton
Copy link
Copy Markdown
Member

nbolton commented Jun 24, 2025

@laz-001 I don't mean to jerk you around with EDataTransfer and EDataReceived. I got confused because we recently removed drag and drop file transfers, and I misremembered the purpose of these. I should have dug a bit deeper!

Copy link
Copy Markdown
Member

@nbolton nbolton left a comment

Choose a reason for hiding this comment

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

So close, just a couple of things I missed last review.

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 942e39d to 1c0ed35 Compare June 24, 2025 20:00
@laz-001

This comment was marked as resolved.

@nbolton
Copy link
Copy Markdown
Member

nbolton commented Jun 24, 2025

So close, just a couple of things I missed last review.

@nbolton , @sithlord48 , please HALT any reviews/comments, something went wrong.

Yeah, I was a bit confused by latest push. I'll hang fire until you say so.

@laz-001
Copy link
Copy Markdown
Contributor Author

laz-001 commented Jun 24, 2025

Yeah, I was a bit confused by latest push. I'll hang fire until you say so.

should be ok now.

@laz-001 I don't mean to jerk you around

The thing is:

  • github PR review is a flawed, inefficient process
    • this gets even worse if the material is textual (articles, docs etc.)

That's why: #8666 (comment)

@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 1c0ed35 to 6ecd3b0 Compare June 24, 2025 20:28
@laz-001 laz-001 force-pushed the i8410-protocol-docs branch from 6ecd3b0 to c9b6dfe Compare June 24, 2025 21:17
@laz-001
Copy link
Copy Markdown
Contributor Author

laz-001 commented Jun 24, 2025

@nbolton , all good from my side. This one needs to go in now, otherwise we end up refining for weeks. Each detail eats a day, further details are surely somewhere in there.

I'm at the point that I need the closure - this is more about 'closing this step' than about the bounty. Doc edit goes on for me once docs are live - that's how I do it usually.

As said, you can commit freely to this branch, I don't even care if I loose authorship (I purge/drop my github accounts anyways regularly).

@sithlord48
Copy link
Copy Markdown
Member

@nbolton , all good from my side. This one needs to go in now, otherwise we end up refining for weeks. Each detail eats a day, further details are surely somewhere in there.

That is fine rather have it take abit longer to land then need a bunch of corrections after. Also makes issues easier to track since its all in one place.

You can commit freely to this branch, I don't even care if I loose authorship (I purge/drop my github accounts anyways regularly).

You just lmk and i will take this over for you so you do not have to deal with the nit picks .

@laz-001
Copy link
Copy Markdown
Contributor Author

laz-001 commented Jun 25, 2025

No, no, I did NOT meant to give this PR even more live.

What i meant to say was: instead of introducing one more day/cycle/iteration, just commit your stuff and merge (instead of waiting for me). E.g. if @nbolton finds something 'final' tomorrow, just commit and merge.

Copy link
Copy Markdown
Member

@nbolton nbolton left a comment

Choose a reason for hiding this comment

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

Finally.

Thanks for persevering!

@nbolton nbolton merged commit 26cc85e into deskflow:master Jun 25, 2025
30 checks passed
@algora-pbc
Copy link
Copy Markdown

algora-pbc bot commented Jun 27, 2025

🎉🎈 @laz-001 has been awarded $200 by Deskflow! 🎈🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants