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

Enhancement, PlotRegion with background #117

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Conversation

R-Abbasi
Copy link
Collaborator

@R-Abbasi R-Abbasi commented Mar 25, 2024

  • set horizontal and vertical gaps around plot region
  • set plot region's background color
  • set border for the region plot
  • fixed image auto resize bug
  • added several code improvements

@AKMaily
Copy link
Collaborator

AKMaily commented Mar 26, 2024

image
It seems that the resizing of the images does not work with every screen. Also the grey background should fill the whole gap between the datawindow and the sidebarmenu as well as the gap between the datawindow and the devices menu.

@R-Abbasi
Copy link
Collaborator Author

Do you mean this way, please?

1

@AKMaily
Copy link
Collaborator

AKMaily commented Mar 27, 2024

Yes the background now looks the way it should.

void SetDevicesMenu(std::map<Omniscope::Id, std::array<float, 3>> &colorMap,
std::optional<OmniscopeSampler> &sampler,
std::vector<std::shared_ptr<OmniscopeDevice>> &devices) {
void SetDevicesMenu(std::map<Omniscope::Id, std::array<float, 3>>& colorMap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

clang format ? Why is there a space between the variable and &

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a (good) habit.
The following constructs are identical with the former potentially making that wrong imagination that the value category of both variables are the same while the latter clarifying the ambiguity making their difference clearer.

T& ri, rj; 
T &ri, rj;

As for clang format, I'm not a big fan of it since I've faced problems in its output, such as:

  • reordering included files (the order of #included files matters)
  • some undesirable optimizations it caries out ending up in changes in the code

As well as, vs-code or VS 2022 code format seems pretty cool.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's okay if you don't like clang-format. We are currently working on an improved version to also check function names and similar elements, so that the C++ standard can be adhered to. You're welcome to incorporate your thoughts here. However, to maintain code structure, we will continue to use the current clang-format file. This also applies to this pull request. Regarding the "&" operator, please utilize the version that clang-format transforms it into.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main point is not my preferences; it's the unwanted consequences that that format may apply - I've encountered them a couple of times in some of the prs already. For example, the code works before pushing but may not on the CI, after applying that format. It can cause easy/hard to find issues.

@AKMaily AKMaily merged commit 7a9ac82 into master Mar 28, 2024
2 checks passed
@AKMaily AKMaily deleted the feature-plot-region branch March 28, 2024 13:20
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.

None yet

2 participants