Page MenuHome

Add new region Footer, and Move Text Editor File Path to the bottom
ClosedPublic

Authored by George Vogiatzis (Gvgeo) on Mar 27 2019, 4:48 PM.

Details

Summary

Added an area 'Footer' as a second header area, and used to display text path.
The area is already setup to be used in other spaces if needed.

Can be hidden with drag.
Can flip top or bottom independent from header.
Use header themes.
Will NOT change color when area gets active.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

With this patch there are too many theme options in preferences.
It is possible to use the colors from header, and avoid creating footer option in all the spaces.

edit:
No idea how to edit or delete inline comments. Just ignore them.
issues resolved with updated D4611

source/blender/editors/screen/screen_ops.c
2317

This case creates changes in rmd->maxsize bellow. Don't know why is needed or how the regions in regions work. (I tested on file browser, without footer)
But If a space(in future) have both footer and region in regions the change in max size will be even more.

  • Either don't include footer here.
  • Or make even more verbose the maxsize calculation below.
  • Or use the simple change below, and have the max size of the region tool props change slightly with the position of the header.
  • Or ignore it for now.

related to this region is D4611 (tried to separate a bit).

2392

The current value places the max size a little lower in case there is both header and footer on top.
And gets smaller with the header in bottom. Will get even smaller if a footer is also bottom.
But don't see a problem in practice, the Y dimension should be big enough to not cause any problems.

I think it's probably fine to re-use the same theme colors in header and footer.

If not, we are really overcrowding our themes unnecessarily I think.

@Brecht Van Lommel (brecht) @Pablo Vazquez (pablovazquez) thoughts?

And if you run it, can you check to see if the solution for the tools properties region is acceptable? With the header up and down change in size (resolution scale 1).

@George Vogiatzis (Gvgeo) Tested, it seems to work correct.

As for your question, I have no idea what you are referring to.

I was referring to the two comments I added in code above.
But that's fine, doesn't matter for now. I don't expect to stay the way it is, right now in this patch.

It's about when you click save as, the options on the lower left panel /* Tool props (aka operator) region */, can be moved up and down.
I changed the maximum height it can go up. And now is also different when the header is up or down.
Don't forget that in other places there are way more options and the window may be smaller.

To me this works great.

My main questions are:

  • Do we really need separate theme entries for headers and footers, or could we just re-use the same theme for both?
  • Is it a good idea or not to add the capability to all editors, if we just intend to actually add it to a handful of them?

I think we might add a footer to Text Editor, Clip Editor & File Browser. Can't think of any other editors where we are likely to add it, but even so, the solution in this patch may be the correct approach.

Adding it as a generic region that all editors can possibly use is fine. For the theme colors I would not add a separate entry, and just reuse the header colors. We already have too many of those.

@George Vogiatzis (Gvgeo) If you make the footer use the header theme colors, we could do final review. Again, thanks for the great contribution.

I tried to solve the problem In the two inline comments, and found bugs.
I updated D4611 with the fixes.
If I update this patch, for a proper review, now will need these fixes first. Or it won't run.

George Vogiatzis (Gvgeo) planned changes to this revision.Mar 29 2019, 8:13 PM

I mentioned the need of a footer region plenty of times, glad it's finally being done.

Seems like all issues have been addressed, but yes, no new theme settings please!

George Vogiatzis (Gvgeo) added inline comments.
source/blender/editors/screen/screen_ops.c
2316

No idea how to edit or delete inline comments. Just ignore them.
issues resolved with updated D4611

The updated version seems good to me, using the header theme.

The updated version seems good to me, using the header theme.

Ops... I just updated the summary and the comments.
Should be still marked as planned changes.
I have to wait for the D4611 to update the diff.

I can't see what D4611 has to do with this task - is there something in there that is blocking this patch from being completed?

@William Reynish (billreynish) As I see it, there is. This patch needs those 3 fixes D4611 to be correct.

I made the 3 fixes in patch, while I was working on footer, because I wanted to add it correctly.
I simply split them, because they are bug fixes and stand on their own.

But I just tried to update this patch, and from what I see, will probably work.
(I could not test it. My git gives me unresolved conflicts.)
The issue(minor) will be, if you add footer to the file browser.
Meanwhile I can update the fixes patch and solve the issue there.

But I would prefer to do in the correct order, if you don't mind.

This revision is now accepted and ready to land.Apr 1 2019, 8:45 PM

Well, ok, it’s just that things can get a bit confusing I think if they are spread out over several patches, but maybe it’s fine.

@Jacques Lucke (JacquesLucke) Can you take a look at these two patches? Thanks

Personally, I'd prefer to see a decision on T61244 before adding footers...

I think the foot appearing on the opposite side of the header is fine. Removing bottom header is a separate discussion, this is trivial to adapt if needed so I see no reason to have it blocked.

I think the foot appearing on the opposite side of the header is fine.

Always be opposite side or just the default setting?
Right now footer, can flip top or bottom independent from header.

George Vogiatzis (Gvgeo) planned changes to this revision.Apr 2 2019, 7:07 PM

Need to fix style.

It's fine as is in the patch I think. Don't really have a preference for this being automatic or not.

As long as the header is at the top and footer as at the bottom by default, I don't really care if you can move them or not. When testing this patch, everything seems to work just fine if you do flip things around, so that's good.

George Vogiatzis (Gvgeo) retitled this revision from Paper Cut T58147: Move Text Editor File Path to the bottom to Add new region Footer, and Move Text Editor File Path to the bottom.Apr 2 2019, 7:45 PM
George Vogiatzis (Gvgeo) updated this revision to Diff 14530.

Fix style.

This revision is now accepted and ready to land.Apr 2 2019, 7:45 PM

Before you start review the code, consider this.

I intend to update the patch with the same diff for this part, after you commit D4611.
Just to make sure there is no conflict.
updated.

~~~~

Trying to be nicer, added couple inline comments to help with the review (although the whole patch is copy-paste).
Ignore the 2 comments marked as done (wish I could delete them).
Keep in mind that I have roughly 4 months in total experience, including python.
As such I might have comment on things you find easy. But if it helps, let me now.

~~~~
Offtopic:
I first started reading blender code about a month ago, but after 2 weeks of no progress, I gave up on blender.
Couple days later @Pablo Vazquez (pablovazquez) mention @Jacques Lucke (JacquesLucke)'s video in monday's stream, and decided to try again using it as a guide.
So... now you know who to blame, if I give you hard time reviewing the code.

source/blender/editors/interface/interface_region_menu_popup.c
293

No many spaces have footer, while ED_area_footer_alignment(sa) fallbacks to RGN_ALIGN_BOTTOM.
In most of the cases, because the alignment is independent, ARegion *ar = CTX_wm_region(C); will fire.
I switched check order for footer, to save 1 check.

source/blender/editors/screen/area.c
1919

When changing editors with wheel, keep footer opposite of header.

At couple places does extra job when checking if a space has also a info bar, instead of only searching for header.
I tried to minimize it but I don't really like it. Maybe should just remove break, and let it check all regions for footer too.

As it is written now, I have a comment in footer definition for this case.

source/blender/editors/screen/screen_ops.c
2320

I intend to update the patch with the same diff for this part, after you commit D4611.
Just to make sure there is no conflict.

source/blender/windowmanager/intern/wm_event_system.c
1404

avoid teleporting mouse on the other side, when moving footer items left-right.

4702

It is annoying to switch the keymap info in the status bar when, you move the mouse over in an other's area header/footer just a bit, by accident.
Or at least, that's what I think, is for.

source/blender/blenloader/intern/versioning_280.c
2964

is it correct, that sa->spacedata.first->regionbase is not "fixed"? I'd think the footer should be added everywhere.

2975

is it important, that the footer region is inserted right after the header?

2978

double semicolon

source/blender/editors/interface/interface_region_menu_popup.c
284

Looks like you could check for ar in the outer if statement as well.

293

for symmetry reasons, the condition order can also be changed for headers.

source/blender/editors/interface/interface_region_popover.c
190

since every case checks for ar, this check can also be moved to an outer condition.

source/blender/editors/screen/area.c
1150

can we expect that footers always have the exact same size as headers?
Maybe having a separate ED_area_footersize is better, even if it internally falls back to ED_area_headersize.

source/blender/editors/screen/screen_ops.c
2471

the original comment talked about the header, is this not correct anymore?

source/blender/makesdna/DNA_screen_types.h
595

should this comment be here?

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 3 2019, 5:11 PM
This revision now requires changes to proceed.Apr 3 2019, 5:11 PM
source/blender/editors/interface/interface_region_menu_popup.c
293

Most spaces now have header on top by default.
The same case as the footer, save 1 check.
leave it or change for symmetry as it is not that important?

source/blender/editors/screen/screen_ops.c
2472

As far as I know it is no possible to hide the header from the edge.

source/blender/editors/interface/interface_region_menu_popup.c
293

Just do what looks the best/cleanest. I don't think this change has any noticeable performance impact.

source/blender/editors/screen/screen_ops.c
2472

I see, looks like you're right.

Yes, dragging headers to close them was removed in 2.8, because many users would do this by mistake. You can still right-click and toggle them on/off, you you can't drag to close them anymore.

Added ED_area_footersize,
and beautify code a bit.

Will add more comments soon.

George Vogiatzis (Gvgeo) marked 9 inline comments as done.Apr 3 2019, 9:12 PM

The two issues I still have are about:
a. ED_area_newspace (see the two inline comments added with this comment).

b. versioning_280.c -> Line 2977

ListBase *regionbase = (sl == sa->spacedata.first) ? &sa->regionbase : &sl->regionbase;

@Jacques Lucke (JacquesLucke)'s comment

is it correct, that sa->spacedata.first->regionbase is not "fixed"? I'd think the footer should be added everywhere.

Looks correct to me. This is what I found.

	/* A list of space links (editors) that were open in this area before. When
	 * changing the editor type, we try to reuse old editor data from this list.
	 * The first item is the active/visible one.
	 */
	/** #SpaceLink. */
	ListBase spacedata;
	/* NOTE: This region list is the one from the active/visible editor (first item in
	 * spacedata list). Use SpaceLink.regionbase if it's inactive (but only then)!
	 */
	/** #ARegion. */
	ListBase regionbase;

I'd think the footer should be added everywhere.

I have no idea what you mean with "everywhere". It is added in every SPACE_TEXT it can find, after header. Even if the Space is inactive. Is there a problem I cannot see?

The whole versioning part is direct copy from the RGN__TYPE_NAV_BAR added in 280, 29.

source/blender/blenloader/intern/versioning_280.c
2975

area.c ->line 1913
Loop in ED_area_newspace() will break when it will find the footer.

source/blender/makesdna/DNA_screen_types.h
595

Removed comment.

I only had it because,
when you add footer in other spacetype, will need to add the spacetype in ED_area_newspace.
Area.c -> line 1904

To me it looks like there are len(sa->spacedata) + 1 region bases in every area.
One in the sa->regionbase and the others in sl->regionbase.

It looks like, you would need to insert the footer in two different region bases, when it is currently active. I could be wrong though. Maybe @Brecht Van Lommel (brecht) or @Julian Eisel (Severin) can tell what's right.

source/blender/blenloader/intern/versioning_280.c
2975

Please don't depend on the order of the regions anywhere if it can be avoided, and there it certainly can (afaik).
Just add the footer region to the beginning or end of the regionbase, except @Brecht Van Lommel (brecht) says that there are any ordering guarantees.

I looked at ED_area_newspace, it does not seem to be the case, it swap regionbase and clears old data.
(there is 1 extra region base, but is unused)

Will fix the footer header order later.

Made footer independent from region order.

Looks good to me now, code wise.

I found one versioning issue. When I'm in the default .blend file, and then switch to the Scripting workspace, the footer is not there.
Can you reproduce that?

I believe the issue is, that every workspace also stores a bScreen.
I just tried simply iterating over the workspaces in the versioning code, but got a segfault. Don't know why that is. Maybe the pointers are not correctly linked when the versioning code runs?
@Brecht Van Lommel (brecht), how should this be done?

Yeah, can confirm. I failed to check that part.
previous diff works fine.
Will try to understand why it stopped working.

All screens are in bmain->screens, so there must be another reason for this. I should not be needed to go from workspace to screen.

Regarding the crash if it would have been needed: datablock pointers are not resolved yet in do_versions() only in do_versions_after_linking().

I found the problem, but cannot say I know the reason.
No region will work after RGN_TYPE_WINDOW.

But with BLI_addhead now will reverse the alignment order in old space areas.

Will revert versioning file to add after header again.

Revert versioning to add footer after header.

Tried couple times the workspaces, but don't get any error.

So much about we don't depend on the order of regions -.-
Will test it again tomorrow, and then we can merge it, hopefully :)

I missed that comment, region order does indeed matter.

Revert and the rest of code, that depends on footer been after header.
(1 break for sync_header_alignment)

As long the footer, always need to be after header for correct alignment, makes no sense to loop everything.

George Vogiatzis (Gvgeo) marked 2 inline comments as done.Apr 4 2019, 8:58 PM

Could not find a case in which it does not work.

@Brecht Van Lommel (brecht), can I merge this?

source/blender/editors/interface/interface_region_menu_popup.c
281

style

This revision is now accepted and ready to land.Apr 5 2019, 1:32 PM

Style:
Add missing space.
And Moved 1 line down. Versioning line 2987

This revision was automatically updated to reflect the committed changes.