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.

Sign up for GitHub

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

Jump to bottom

Reworked the book/tutorial #1379

Merged
merged 9 commits into from
Feb 9, 2023
Merged

Reworked the book/tutorial #1379

merged 9 commits into from
Feb 9, 2023

Conversation

pentamassiv
Copy link
Contributor

A while ago I created a draft PR ( #1354) to improve the book. A good number of commits were done since then. I am not that familiar with git and I think now the PR is ready, so I created this PR.
A few months have passed and I didn't find the time to add the missing information to explain the whole process of how to create the bindings for pango. The overhaul contains all the information of the current version and adds some to it. I believe this version can already be merged and is already an improvement.
To recap, the improvements include:

  • Restructuring the book a little bit to make it easier to find information
  • Adding description on what gir is and when it can be used
  • Explanation on how to set up the project folder
  • Explanation on how to find .gir files
  • Mentioning of various other foot guns that a beginner might encounter

A native speaker should probably proofread it.

This was referenced Sep 21, 2022
Closed
Closed
Closed
@pentamassiv
Copy link
Contributor Author

@bilelmoussaoui do you maybe have some input for this PR as well?

@bilelmoussaoui
Copy link
Member

@bilelmoussaoui do you maybe have some input for this PR as well?

I will review this tomorrow morning

@bilelmoussaoui bilelmoussaoui requested review from bilelmoussaoui and removed request for bilelmoussaoui October 8, 2022 11:37
@pentamassiv
Copy link
Contributor Author

Rebased

@pentamassiv
Copy link
Contributor Author

Do you have some feedback Bilal?

@bilelmoussaoui
Copy link
Member

Do you have some feedback Bilal?

Sorry for taking too long, I have been busy with finishing up the release. I will get back to this as soon as possible

@pentamassiv
Copy link
Contributor Author

Yes, of course. I didn't want to be impolite.
Thank you very much

@pentamassiv
Copy link
Contributor Author

Rebased

bilelmoussaoui
bilelmoussaoui reviewed Nov 1, 2022
View reviewed changes
Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

A first round of reviews, haven't had the time/energy to go through all of it yet but will get back to it in the upcoming days, thanks a lot!

book/src/config/ffi.md Show resolved Hide resolved
book/src/introduction.md Outdated Show resolved Hide resolved
book/src/introduction.md Outdated Show resolved Hide resolved
book/src/introduction.md Outdated Show resolved Hide resolved
book/src/introduction.md

- `normal`: generates another crate for a layer on top of these unsafe (sys) bindings which makes them safe for use in general Rust.

- `not_bound`: allows you to see the detected types/methods that will not be generated for whatever reasons.
Copy link
Member

Choose a reason for hiding this comment

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

The not_bound mode was dropped from the docs, it should be still mentioned as it is pretty useful when you are working on a new library bindings or if the library changes a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mentioned in lines 15 & 16 in the same file. Do you want me to rephrase it?

book/src/tutorial/generate_docs.md Outdated Show resolved Hide resolved
book/src/tutorial/introduction.md Outdated Show resolved Hide resolved
book/src/tutorial/preparation.md Show resolved Hide resolved
book/src/tutorial/sys_library.md Outdated Show resolved Hide resolved
book/src/tutorial/sys_library.md Outdated Show resolved Hide resolved
@pentamassiv pentamassiv mentioned this pull request Nov 1, 2022
Open
@pentamassiv
Copy link
Contributor Author

I made changes to address all suggestions. For a few, I need some feedback to fix it.

@pentamassiv
Copy link
Contributor Author

@bilelmoussaoui, did you get a chance to review the rest?

@bilelmoussaoui
Copy link
Member

@bilelmoussaoui, did you get a chance to review the rest?

I will have a look this weekend

@bilelmoussaoui bilelmoussaoui self-requested a review November 30, 2022 16:47
Hofer-Julian
Hofer-Julian reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

I had a quick look for spelling, not for content.

Not specific to this PR, but I would recommend adding a newline after each sentence.
It doesn't change the output, but makes diffs much more readable in my opinion.

book/src/introduction.md Outdated Show resolved Hide resolved
book/src/introduction.md Outdated Show resolved Hide resolved
book/src/tutorial/finding_gir_files.md Outdated Show resolved Hide resolved
book/src/tutorial/high_level_rust_api.md Outdated Show resolved Hide resolved
book/src/tutorial/preparation.md Outdated Show resolved Hide resolved
book/src/tutorial/sys_library.md Outdated Show resolved Hide resolved
@pentamassiv
Copy link
Contributor Author

Rebased on the latest master

@pentamassiv
Copy link
Contributor Author

Thank you @Hofer-Julian for the suggestions. I have applied all of them. There are currently no other pending suggestions that I am aware of.

@bilelmoussaoui bilelmoussaoui mentioned this pull request Feb 6, 2023
Closed
pentamassiv and others added 5 commits February 9, 2023 09:54
@pentamassiv @sdroege
Reworked the book/tutorial
07fc701
@pentamassiv @bilelmoussaoui @sdroege
Applied Bilals suggestions
f8a13a7 
Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>

Co-authored-by: Bilal Elmoussaoui <[email protected]>
@pentamassiv @sdroege
Add information about the dox feature
7991951
@pentamassiv @sdroege
Don't make the submodules auto update
7c08952
@pentamassiv @sdroege
Add gir-files as submodule
03e2698
@sdroege sdroege merged commit 43b9242 into gtk-rs:master Feb 9, 2023
@pentamassiv
Copy link
Contributor Author

Thank you for merging it :)

@sdroege
Copy link
Member

sdroege commented Feb 9, 2023

Sorry for taking so long to review it

@pentamassiv
Copy link
Contributor Author

No problem. I understand that it was a lot to read and go through and probably not of the highest priority

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

@Hofer-Julian Hofer-Julian Hofer-Julian left review comments

@sdroege sdroege sdroege approved these changes

@GuillaumeGomez GuillaumeGomez Awaiting requested review from GuillaumeGomez

@bilelmoussaoui bilelmoussaoui Awaiting requested review from bilelmoussaoui

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pentamassiv @bilelmoussaoui @sdroege @Hofer-Julian