Skip to content

Feature 94#173

Closed
wdednam wants to merge 2 commits into
highperformancecoder:masterfrom
wdednam:feature-94
Closed

Feature 94#173
wdednam wants to merge 2 commits into
highperformancecoder:masterfrom
wdednam:feature-94

Conversation

@wdednam
Copy link
Copy Markdown
Contributor

@wdednam wdednam commented Apr 15, 2020

For some reason the setters for width() and height() I added in the RenderVariable class do not change the values of the private members w and h of that class. I made them mutable but this still didn't make a difference. moveTo in VariableBase::resize() seems to work though...


This change is Reviewable

@highperformancecoder
Copy link
Copy Markdown
Owner

I think this is the wrong move. RenderVariable/ RenderOperation are about rendering and measuring the space onscreen icons take. The width() and height() attributes are not editable - they are calculated values depending on the variable or operation they are constructed from. In some ways, these classes ought to be folded back into the regular Item classes, which has the BoundingBox element performing the same function, but that would be a further refactoring exercise, that is probably not warranted at the present time.

What you need to do is merge something like the sheet implementation into the general Item class implementation. Things which are currently resizable are GodleyIcons, PlotWidgets, Ravels and Sheets. They should all be refactored to use the new method where possible - Sheet being the latest, and probably simplest implementation.

@wdednam
Copy link
Copy Markdown
Contributor Author

wdednam commented Apr 16, 2020

I understand, thanks.I guess I should take the most general case and then use if statements or a switch to specialize to different canvas objects.

@highperformancecoder
Copy link
Copy Markdown
Owner

highperformancecoder commented Apr 16, 2020 via email

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.

2 participants