UI: Clicking the camera icon while rendering should open render window #105765

Rémy Fayet · 2023-03-14T15:49:07+01:00

Rémy Fayet commented

2023-03-14 15:49:07 +01:00

During rendering, clicking on the Camera icon of the jobs panel should focus or open a Render Window.

Adds a op_name variable to store the operation.
Fix #102210

During rendering, clicking on the Camera icon of the jobs panel should focus or open a Render Window. Adds a `op_name` variable to store the operation. Fix #102210

👍 1

Rémy Fayet added 1 commit 2023-03-14 15:49:17 +01:00

747f24472d

UI: Clicking the camera icon while rendering should open render window

During rendering, clicking on the Camera icon of the jobs panel should focus or open a Render Window.

Adds a `op_name` variable to store the operation.
Fix #102210

Iliya Katushenock added this to the User Interface project 2023-03-14 15:50:27 +01:00

Harley Acheson commented

2023-03-15 01:14:15 +01:00

This is a nice idea. Thanks for working on it. I can't think of any good reason why we couldn't have this. But I have a few suggestions:

If op_name is anything at all then it is going to show a tooltip that it is "Show/ focus render window." Your change will be more generic and usable for other job types if you deal with the description in the exact same way as you do with op_name, perhaps "op_desc"?

In our C++ files (those ending in ".cc") our coding style is to use nullptr rather than NULL.

The tooltip description should be a plain and regular English sentence as that is clearer and easier to translate, so I would find an alternative without the slash. Perhaps just "Show the render window"?

Your change adds an "if" and "else" that largely duplicates the uiDefIconTextBut call. You could get rid of the "else" and its contents. Then the icon argument for the current uiDefIconTextBut could be a ternary like (op_name) ? 0 : icon. Would make it simpler.

This is a nice idea. Thanks for working on it. I can't think of any good reason why we couldn't have this. But I have a few suggestions: If op_name is anything at all then it is going to show a tooltip that it is "Show/ focus render window." Your change will be more generic and usable *for other job types* if you deal with the description in the exact same way as you do with op_name, perhaps "op_desc"? In our C++ files (those ending in ".cc") our coding style is to use `nullptr` rather than `NULL`. The tooltip description should be a plain and regular English sentence as that is clearer and easier to translate, so I would find an alternative without the slash. Perhaps just "Show the render window"? Your change adds an "if" and "else" that largely duplicates the `uiDefIconTextBut` call. You could get rid of the "else" and its contents. Then the icon argument for the current `uiDefIconTextBut` could be a ternary like `(op_name) ? 0 : icon`. Would make it simpler.

Rémy Fayet added 1 commit 2023-03-15 12:53:42 +01:00

1f8dc7b7ae

Match cpp coding style and remove duplicates

Rémy Fayet commented

2023-03-15 12:56:27 +01:00

Thank you for your feedback. I think I have corrected all the points you mentioned.

Harley Acheson commented

2023-03-18 02:29:31 +01:00

@rems64 - Thank you for your feedback. I think I have corrected all the points you mentioned.

No worries. That looks really nice, and seems to be just what Jeroen was looking for.

> @rems64 - Thank you for your feedback. I think I have corrected all the points you mentioned. No worries. That looks really nice, and seems to be just what Jeroen was looking for.

Harley Acheson requested review from Jeroen Bakker 2023-03-18 02:29:55 +01:00

Jeroen Bakker reviewed 2023-03-18 10:02:20 +01:00

source/blender/editors/interface/interface_templates.cc Outdated

						
				@ -6125,2 +6125,4 @@

				  void *owner = nullptr;

				  int handle_event, icon = 0;

				  const char *op_name = nullptr;

				  const char *op_desc = nullptr;

Jeroen Bakker commented

2023-03-18 09:58:35 +01:00

Best to use none abbreviated names op_description.
Code is mostly written once, but read many times, code and naming should reflect this.

Best to use none abbreviated names `op_description`. Code is mostly written once, but read many times, code and naming should reflect this.

Jeroen Bakker commented

2023-03-18 10:03:44 +01:00

Note: I didn't test drive it as it is still marked as WIP. If you want a proper review remove the WIP, so I known the state of the PR.

Rémy Fayet added 1 commit 2023-03-18 14:24:04 +01:00

441a043b71

Remove abbreviation

Rémy Fayet added 1 commit 2023-03-18 14:25:03 +01:00

1d7ed8275f Merge branch 'main' into T102210-click-camera-icon

Rémy Fayet changed title from ~~WIP: UI: Clicking the camera icon while rendering should open render window~~ to UI: Clicking the camera icon while rendering should open render window

2023-03-18 14:25:17 +01:00

Jeroen Bakker requested changes 2023-03-20 09:48:32 +01:00

Jeroen Bakker left a comment

Patch doesn't work in all cases of Temporary Windows Perhaps we should only add the operation when the Temporary Editors setting makes sense. Eg Keep User Interface doesn't work, but I would also not expect it to be able to work in a sense that the user wants it to work. Other options should be supported.

Patch doesn't work in all cases of `Temporary Windows` Perhaps we should only add the operation when the `Temporary Editors` setting makes sense. Eg `Keep User Interface` doesn't work, but I would also not expect it to be able to work in a sense that the user wants it to work. Other options should be supported.

Rémy Fayet commented

2023-03-20 21:48:33 +01:00

Oops, I forgot to remove the automatic WIP, I still have to get used to it.
Is it better to exclude the Keep User Interface case, or to explicitly specify which temporary windows should display the button? I chose the first option, but does it really fit with Blender's coding style? A small function like display_type_show_but or an explicit switch statement with each temporary window setting seems to be more widely used in such cases.

Oops, I forgot to remove the automatic WIP, I still have to get used to it. Is it better to exclude the `Keep User Interface` case, or to explicitly specify which temporary windows should display the button? I chose the first option, but does it really fit with Blender's coding style? A small function like `display_type_show_but` or an explicit switch statement with each temporary window setting seems to be more widely used in such cases.

Rémy Fayet added 2 commits 2023-03-20 21:53:42 +01:00

a08ee75fd3

Exclude the `Keep User Interface` case

b2265dd155

Merge branch 'T102210-click-camera-icon' of projects.blender.org:rems64/blender into T102210-click-camera-icon

Jeroen Bakker approved these changes 2023-03-21 07:47:06 +01:00

Jeroen Bakker commented

2023-03-21 07:51:42 +01:00

although a switch statement would be preferred, it has to make sense. In this case a switch-statement doesn't and an exclusive if statement is fine (like you did). USER_RENDER_DISPLAY_NONE is used in other places as an early exit.

Jeroen Bakker added this to the 3.6 LTS milestone 2023-03-21 07:52:03 +01:00

Jeroen Bakker merged commit 439beb20c3 into main

2023-03-21 07:53:41 +01:00

Jeroen Bakker referenced this issue from a commit

2023-03-21 07:53:43 +01:00

UI: Clicking camera icon while rendering should open render window

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

UI: Clicking the camera icon while rendering should open render window #105765