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

Several code cleanups and refactorings #618

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Several code cleanups and refactorings #618

merged 4 commits into from
Oct 24, 2023

Conversation

psaavedra
Copy link
Member

These changes are motivated towards the progress of being able to have more than one view:

  • Move declaration of CogWlPlatform to a header file
  • More fix code-style in cog-platform-wl.c
  • Encapsulate the seat resources inside of CogWlSeat. All the related resources to an individual wl_seat are encapsulated into a CogWlSeat struct. The CogWlDisplay contains a CogWlSeat (the first registered).
  • Redefine wl_data and egl_data as CogWlDisplay. The CogWlDisplay is defined in cog-utils-wl.h. Also wl_data and egl_data definition are renamed as s_display.

@psaavedra psaavedra self-assigned this Oct 20, 2023
@psaavedra psaavedra changed the title Psaavedra/main refactorings Several code cleanups and refactorings Oct 20, 2023
@aperezdc
Copy link
Member

@psaavedra This needs a rebase now that #617 has already been landed, could you please update it? Thanks!

aperezdc
aperezdc requested changes Oct 20, 2023
View reviewed changes
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

Needs rebasing.

@psaavedra
Copy link
Member Author

Needs rebasing.

Yes, sorry for that. Done.

@psaavedra psaavedra mentioned this pull request Oct 22, 2023
Merged
@psaavedra
Copy link
Member Author

Rebased against master after #605 being landed. @aperezdc please r?.

aperezdc
aperezdc requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

I have a couple of small nits, otherwise the patch LGTM and we could merge it when the small comments are addressed. Thanks for the cleanup!

platform/wayland/cog-platform-wl.h Outdated
Comment on lines 9 to 10
#ifndef COG_PLATFORM_WL_PLATFORM_H
#define COG_PLATFORM_WL_PLATFORM_H
Copy link
Member

Choose a reason for hiding this comment

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

We prefer #pragma once these days:

Suggested change
#ifndef COG_PLATFORM_WL_PLATFORM_H
#define COG_PLATFORM_WL_PLATFORM_H
#pragma once

Then you won't need the #endif below either.

platform/wayland/cog-platform-wl.h Outdated
Comment on lines 27 to 28

#endif /* !COG_PLATFORM_WL_PLATFORM_H */
Copy link
Member

Choose a reason for hiding this comment

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

No need for this #endif when using #pragma once, and the G_END_DECLS needs to go here:

Suggested change
#endif /* !COG_PLATFORM_WL_PLATFORM_H */
G_END_DECLS

platform/wayland/cog-platform-wl.h Outdated
G_BEGIN_DECLS

G_DECLARE_FINAL_TYPE(CogWlPlatform, cog_wl_platform, COG, WL_PLATFORM, CogPlatform)
G_END_DECLS
Copy link
Member

Choose a reason for hiding this comment

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

Function and struct declarations also need to be inside G_{BEGIN,END}_DECLS, this needs to be moved to the end of the file.

Suggested change
G_END_DECLS

platform/wayland/cog-utils-wl.h
struct _CogWlSeat {
struct wl_seat *seat;
uint32_t seat_name;
uint32_t seat_version;
Copy link
Member

Choose a reason for hiding this comment

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

Funny thing: I just realized that seat_version is not used anywhere. Let's leave it be for now, but we could make a follow-up patch later on to remove it 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I am preparing another change for supporting multiple seats. I will keep this in mind.

@psaavedra
wl: Move declaration of CogWlPlatform to a header file
2173571
@psaavedra
wl: Redefine wl_data and egl_data as CogWlDisplay
464acda 
The CogWlDisplay is defined in cog-utils-wl.h. Also wl_data and
egl_data definition are renamed as s_display.
@psaavedra
wl: Encapsulate the seat resources inside of CogWlSeat
e736b76 
All the related resources to an individual wl_seat are encapsulated into
a CogWlSeat struct. The CogWlDisplay contains a CogWlSeat (the first
registered).

This change is motivated towards the progress of being able to have more
than one view.
@psaavedra
wl: Fix code-style in cog-platform-wl.c
9781b90
aperezdc
aperezdc approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

This is now ready for landing 🛬

@aperezdc aperezdc merged commit 7158e8e into master Oct 24, 2023
5 checks passed
@aperezdc aperezdc deleted the psaavedra/main-refactorings branch October 24, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aperezdc aperezdc aperezdc approved these changes

Assignees

@psaavedra psaavedra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@psaavedra @aperezdc