Implementation of new blendfile compatibility policy #110109

Closed
Bastien Montagne wants to merge 16 commits from mont29:F-109151 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

This PR makes the following changes:

  • The minfileversion stored in Blender files is now a hard limit,
    Blender does not open any file which minfileversion is newer than its
    current version.
    • An error message is shown to user when trying to open such file.
    • When trying to browse such file (for linking or appending), the
      FileBrowser shows an error message instead of the content of the
      library file.
  • When a newer fileversion blendfile is open, a permanent warning is
    shown in the footer of the UI.
    • By clicking on it, user can get more info.
    • When trying to save such newer file, a popup asks for confirmation,
      with default option to 'Save As' instead.
    • If 'save confirmation' option is enabled, when quiting or opening
      another file, and current one is newer, the 'Save' option is replaced
      by a 'Save As' one.

Technically, the hard check on minfileversion is implemented as a new
util in readfile code, blo_check_minversion, which is called as part
of the process to get a FileData (aka BlendHandle) from a file path.

The 'soft warning' about data loss due to forward compatibility
limitations is stored as a new flag in Main,
has_forward_compatibility_issues.

NOTE: Final commit to main (and backports to both LTSs) will happen as
several commits.

Implements #109151.

PS: Attached two blend files, artificially created from Blender 5.0, and Blender 4.50, for easier testing ;)

This PR makes the following changes: * The minfileversion stored in Blender files is now a hard limit, Blender does not open any file which minfileversion is newer than its current version. * An error message is shown to user when trying to open such file. * When trying to browse such file (for linking or appending), the FileBrowser shows an error message instead of the content of the library file. * When a newer fileversion blendfile is open, a permanent warning is shown in the footer of the UI. * By clicking on it, user can get more info. * When trying to save such newer file, a popup asks for confirmation, with default option to 'Save As' instead. * If 'save confirmation' option is enabled, when quiting or opening another file, and current one is newer, the 'Save' option is replaced by a 'Save As' one. Technically, the hard check on minfileversion is implemented as a new util in readfile code, `blo_check_minversion`, which is called as part of the process to get a FileData (aka BlendHandle) from a file path. The 'soft warning' about data loss due to forward compatibility limitations is stored as a new flag in Main, `has_forward_compatibility_issues`. NOTE: Final commit to main (and backports to both LTSs) will happen as several commits. Implements #109151. PS: Attached two blend files, artificially created from Blender 5.0, and Blender 4.50, for easier testing ;)
Bastien Montagne added 1 commit 2023-07-14 16:35:09 +02:00
WIP: Initial implementation of new blendfile compatibility policy.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
584ad89079
This PR makes the following changes:
* The minfileversion stored in Blender files is now a hard limit,
  Blender does not open any file which minfileversion is newer than its
  current version.
** An error message is shown to user when trying to open such file.
** When trying to browse such file (for linking or appending), the
   FileBrowser shows an error message instead of the content of the
   library file.
* When a newer fileversion blendfile is open, a permanent warning is
  shown in the footer of the UI.
** By clicking on it, user can get more info.
** When trying to save such newer file, a popup asks for confirmation,
   with default option to 'Save As' instead.
** If 'save confirmation' option is enabled, when quiting or opening
   another file, and current one is newer, the 'Save' option is replaced
   by a 'Save As' one.

Technically, the hard check on minfileversion is implemented as a new
util in readfile code, `blo_check_minversion`, which is called as part
of the process to get a FileData (aka BlendHandle) from a file path.

The 'soft warning' about data loss due to forward compatibility
limitations is stored as a new flag in Main,
`has_forward_compatibility_issues`.

NOTE: Final commit to main (and backports to both LTSs) will happen as
several commits.

Implements #109151.
Bastien Montagne added the
Module
Core
Interest
BlendFile
labels 2023-07-14 16:35:28 +02:00
Author
Owner

@dfelinto @brecht @Sergey @ideasman42 Not sure who wants (and has time) to review this?

@dfelinto @brecht @Sergey @ideasman42 Not sure who wants (and has time) to review this?
Author
Owner

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110109) when ready.
Sergey Sharybin reviewed 2023-07-14 16:57:38 +02:00
Sergey Sharybin left a comment
Owner

Just some naming suggestions to solve ambiguity.

Overall on a code side seems fine, but would we'll need to check the interface and wording part of it.

Just some naming suggestions to solve ambiguity. Overall on a code side seems fine, but would we'll need to check the interface and wording part of it.
@ -44,0 +50,4 @@
* 8, or 12 if subversion is provided. \param file_subversion the file subversion, if given value <
* 0, it is ignored, and only the `file_version` is used. */
void BKE_blender_version_blendfile_string_from_values(char *str_buff,
const size_t str_buff_len,

Is it the size of the buffer which needs to be 8, or the length (which makes it a requirement of the buffer size of 9) ?

Is it the size of the buffer which needs to be 8, or the length (which makes it a requirement of the buffer size of 9) ?
mont29 marked this conversation as resolved
@ -1080,1 +1083,4 @@
/** Check if minversion of the file is older than current Blender, return false if it is not.
* Should only be called after #read_file_dna was successfuly executed. */
static bool blo_check_minversion(FileData *fd, ReportList *reports)

The "check" semantic of functions is hard to understand by themselves. Having a function which "check condition" and return false if the condition is true makes it even harder.

Not sure the blo_ prefix helps here, so is_minversion_older_than_blender will be very hard to misread when reading code later on if (is_minversion_older_than_blender(fd, reports)) { ... }

The "check" semantic of functions is hard to understand by themselves. Having a function which "check condition" and return false if the condition is true makes it even harder. Not sure the `blo_` prefix helps here, so `is_minversion_older_than_blender` will be very hard to misread when reading code later on `if (is_minversion_older_than_blender(fd, reports)) { ... } `
mont29 marked this conversation as resolved
Bastien Montagne added 2 commits 2023-07-14 18:11:03 +02:00
Updates from review.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
345f399585
Author
Owner

Just some naming suggestions to solve ambiguity.

Thanks, addressed.

Overall on a code side seems fine, but would we'll need to check the interface and wording part of it.

Definitely.

> Just some naming suggestions to solve ambiguity. Thanks, addressed. > Overall on a code side seems fine, but would we'll need to check the interface and wording part of it. Definitely.
Author
Owner

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110109) when ready.
Brecht Van Lommel requested changes 2023-07-18 18:41:41 +02:00
@ -458,6 +465,10 @@ int set_listbasepointers(struct Main *main, struct ListBase *lb[]);
((main)->versionfile < (ver) || \
((main)->versionfile == (ver) && (main)->subversionfile < (subver)))
#define MAIN_VERSION_OLDER_OR_EQUAL(main, ver, subver) \

MAIN_VERSION_OLDER_OR_EQUAL -> MAIN_VERSION_FILE_OLDER_OR_EQUAL for clarity?

My first guess without looking at the implementation was that this was not about the file version.

MAIN_VERSION_OLDER_OR_EQUAL -> MAIN_VERSION_FILE_OLDER_OR_EQUAL for clarity? My first guess without looking at the implementation was that this was not about the file version.
Author
Owner

This follows naming of existing MAIN_VERSION_ATLEAST and MAIN_VERSION_OLDER.

If we want to state these are for file version, all need to be cleanedup first (which I don't mind doing at all, actually ;) ).

This follows naming of existing `MAIN_VERSION_ATLEAST` and `MAIN_VERSION_OLDER`. If we want to state these are for file version, all need to be cleanedup first (which I don't mind doing at all, actually ;) ).
mont29 marked this conversation as resolved
@ -128,0 +138,4 @@
"%d.%d.%d",
file_version_major,
file_version_minor,
file_subversion);

This displays the subversion as a patch version, but they are not the same.

Not sure what the best way to show it is, we can invent whatever syntax is different enough.

This displays the subversion as a patch version, but they are not the same. Not sure what the best way to show it is, we can invent whatever syntax is different enough.
Author
Owner

Replaced the . by a dash for the last (subversion) number.

Replaced the `.` by a `dash` for the last (subversion) number.

A dash is still pretty a relative subtle difference. Since it's only for development versions, perhaps something more verbose like this?

%d.%d (sub %d)
A dash is still pretty a relative subtle difference. Since it's only for development versions, perhaps something more verbose like this? ``` %d.%d (sub %d) ```
Author
Owner

Was trying to not be too verbose, but sure, let's go for it then :)

Was trying to not be too verbose, but sure, let's go for it then :)
mont29 marked this conversation as resolved
@ -128,0 +141,4 @@
file_subversion);
}
else {
BLI_snprintf(str_buff, str_buff_len, "%d.%d", file_version_major, file_version_minor);

The subversion will almost always be non-zero, certainly for official releases. Just show it always?

The subversion will almost always be non-zero, certainly for official releases. Just show it always?
Author
Owner

Actually not, because in most cases (when major + minor versions are different), code passes the 'ignore' -1 value for subversions, to avoid cluttering message to user with info that is not really exposed to them anywhere else 9since they typically never see the file subversion info anymore).

So file subversion will typically only be shown when using builds from in-development main/release branches, when opening a file from a later build of the same current release (and file subversion has changed in-between).

Actually not, because in most cases (when major + minor versions are different), code passes the 'ignore' `-1` value for subversions, to avoid cluttering message to user with info that is not really exposed to them anywhere else 9since they typically never see the file subversion info anymore). So file subversion will typically only be shown when using builds from in-development main/release branches, when opening a file from a later build of the same current release (and file subversion has changed in-between).
brecht marked this conversation as resolved
@ -1080,1 +1083,4 @@
/** Check if minversion of the file is older than current Blender, return false if it is not.
* Should only be called after #read_file_dna was successfuly executed. */
static bool is_minversion_older_than_blender(FileData *fd, ReportList *reports)

I guess it's strictly older or equal. Could be named is_minversion_newer_than_blender and return value flipped if we want to brief.

I guess it's strictly older or equal. Could be named `is_minversion_newer_than_blender` and return value flipped if we want to brief.
Author
Owner

The name is actually fine (it does a strictly 'older than' check), but returned values were reversed compared to documentation and expected logical behavior. Thanks for catching this!

The name is actually fine (it does a strictly 'older than' check), but returned values were reversed compared to documentation and expected logical behavior. Thanks for catching this!
mont29 marked this conversation as resolved
@ -1081,0 +1118,4 @@
MEM_freeN(fg);
return false;
}
MEM_freeN(fg);

Break to avoid going through the entire file? Or can this exist multiple times?

Break to avoid going through the entire file? Or can this exist multiple times?
Author
Owner

Indeed

Indeed
mont29 marked this conversation as resolved
@ -1255,0 +1323,4 @@
}
BKE_reports_clear(&sfile->runtime->reports);
if (!BKE_blendfile_is_readable(blendfile_path, &sfile->runtime->reports)) {

Is this opening the file on every redraw? That seems like it could be a performance problem, especially with network drivers where there might be significant latency.

Can this be cached, like I assume other file data show in the file browser is?

Is this opening the file on every redraw? That seems like it could be a performance problem, especially with network drivers where there might be significant latency. Can this be cached, like I assume other file data show in the file browser is?
Author
Owner

Eeeeek, good catch!

Eeeeek, good catch!
mont29 marked this conversation as resolved
Bastien Montagne added 4 commits 2023-07-20 12:12:32 +02:00
Writing this file in specific places is not the responsibility of that
utils module code. this is for caller's logic to handle (in this case,
mainly the update UI translations add-on).
Bastien Montagne requested review from Brecht Van Lommel 2023-07-20 12:13:42 +02:00
Brecht Van Lommel requested changes 2023-07-20 13:48:46 +02:00
Brecht Van Lommel left a comment
Owner

I'm fine with this now besides the one comment.

Maybe @dfelinto or @pablovazquez want to check if the UI matches their suggestions from the design task.

I'm fine with this now besides the one comment. Maybe @dfelinto or @pablovazquez want to check if the UI matches their suggestions from the design task.
Bastien Montagne added 2 commits 2023-07-20 14:33:17 +02:00
Bastien Montagne added 1 commit 2023-07-20 14:34:59 +02:00
Merge branch 'main' into F-109151
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
4ba58003f6
Author
Owner

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110109) when ready.
Brecht Van Lommel approved these changes 2023-07-20 14:41:48 +02:00
  1. I think the loaading error message is too long now, my suggestion:

image

Text:

  • Loading failed: The file was saved with a newer version, open it with Blender 5.0 or later.
  • Loading failed: The file was saved with a newer version, open it with Blender 5.0 (sub 3) or later.

Note that I think "sub" should never really be exposed to users. But this error case (incompatibility between the same release without a major version bump) will never happen anyways outside development versions so it is fine.


  1. I would use a tooltip for the message that shows when you opened a newer Blender. Right now it only shows when you click on the question mark. As a reference, in Geometry Nodes we have those info/warnings on the node, and for more info users mouseover it.

And I would stick to the shorter original proposed message or a variation of it: "File saved by newer Blender (4.0), expect loss of data!".

1) I think the loaading error message is too long now, my suggestion: ![image](/attachments/2efd75d5-f052-4515-8442-3b628b61e89f) Text: * Loading failed: The file was saved with a newer version, open it with Blender 5.0 or later. * Loading failed: The file was saved with a newer version, open it with Blender 5.0 (sub 3) or later. Note that I think "sub" should never really be exposed to users. But this error case (incompatibility between the same release without a major version bump) will never happen anyways outside development versions so it is fine. ---------------- 2) I would use a tooltip for the message that shows when you opened a newer Blender. Right now it only shows when you click on the question mark. As a reference, in Geometry Nodes we have those info/warnings on the node, and for more info users mouseover it. And I would stick to the shorter original proposed message or a variation of it: "File saved by newer Blender (4.0), expect loss of data!".
Author
Owner

Text:

  • Loading failed: The file was saved with a newer version, open it with Blender 5.0 or later.
  • Loading failed: The file was saved with a newer version, open it with Blender 5.0 (sub 3) or later.

Shortened the message, note that putting the full blendfile path here is done for all reports by default, since ages (already there in commit from 2016). So changing this should not be part of this PR imho.

> Text: > * Loading failed: The file was saved with a newer version, open it with Blender 5.0 or later. > * Loading failed: The file was saved with a newer version, open it with Blender 5.0 (sub 3) or later. Shortened the message, note that putting the full blendfile path here is done for all reports by default, since ages (already there in commit from 2016). So changing this should not be part of this PR imho.
Bastien Montagne added 1 commit 2023-07-20 16:58:58 +02:00
Address review comments re UI/layout.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
06e2feee5e
* Remove 'v' in from of versions
* Adjust layout of new 'save confirmation' popup
* Remove 'compatibility warning' operator, replace with basic tootlip
  for now.
* Shorten report about too new files, add more detailed CLOG warning
  instead.
Author
Owner

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110109) when ready.

We need to remove the exclamation point, since it conflicts with the .:
image

Sub is still showing, even though I'm not on Blender 5.0:
image

When closing Blender I think we should say "could cause loss of data" (instead of may). Which would explain why there is no "Save" or overwrite option:

image

We need to remove the exclamation point, since it conflicts with the `.`: ![image](/attachments/f1aacba9-18de-4fcd-baf9-dd8274c12930) Sub is still showing, even though I'm not on Blender 5.0: ![image](/attachments/b97cdcda-3f05-41ec-b93f-ee0ded9dd69d) When closing Blender I think we should say "could cause loss of data" (instead of may). Which would explain why there is no "Save" or overwrite option: ![image](/attachments/37bfc786-4e4d-492e-8d46-f598ed116944)
Bastien Montagne added 1 commit 2023-07-21 11:53:07 +02:00
Author
Owner

We need to remove the exclamation point, since it conflicts with the .:
image

Indeed, will fix.

Sub is still showing, even though I'm not on Blender 5.0:
image

Well yes, it's still the same issue, this specific test file says it was saved with Blender 5 sub 7, and its minversion is 5 sub 3, so subversion is needed here.
Again, in real files this should never happen, since we do not expect ever a 5.0 sub x file to also have a 5.0 sub y minversion.

When closing Blender I think we should say "could cause loss of data" (instead of may). Which would explain why there is no "Save" or overwrite option:

afaik 'could' and 'may' have the same meaning here?

> We need to remove the exclamation point, since it conflicts with the `.`: > ![image](/attachments/f1aacba9-18de-4fcd-baf9-dd8274c12930) Indeed, will fix. > Sub is still showing, even though I'm not on Blender 5.0: > ![image](/attachments/b97cdcda-3f05-41ec-b93f-ee0ded9dd69d) Well yes, it's still the same issue, this specific test file says it was saved with Blender 5 sub 7, and its minversion is 5 sub 3, so subversion is needed here. Again, in real files this should never happen, since we do not expect ever a 5.0 sub x file to also have a 5.0 sub y minversion. > When closing Blender I think we should say "could cause loss of data" (instead of may). Which would explain why there is no "Save" or overwrite option: afaik 'could' and 'may' have the same meaning here?
Bastien Montagne added 1 commit 2023-07-21 12:09:26 +02:00
Dalai Felinto approved these changes 2023-07-21 15:29:12 +02:00
Bastien Montagne added 1 commit 2023-07-23 20:16:29 +02:00
Merge branch 'main' into F-109151
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
6ef3530c08
Author
Owner

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110109) when ready.
Bastien Montagne changed title from WIP: Initial implementation of new blendfile compatibility policy. to Implementation of new blendfile compatibility policy. 2023-07-25 12:18:41 +02:00
Pablo Vazquez approved these changes 2023-07-25 15:19:17 +02:00
Julian Eisel requested changes 2023-07-26 17:55:29 +02:00
Julian Eisel left a comment
Member

Mostly small things, generally looking fine. One potentially bigger change, but I'm not convinced it's worth it, see inline comment.

  • When a newer fileversion blendfile is open, a permanent warning is shown in the footer of the UI.
    • By clicking on it, user can get more info.

This doesn't seem to be implemented? I don't see code to handle this.

Mostly small things, generally looking fine. One potentially bigger change, but I'm not convinced it's worth it, see inline comment. > * When a newer fileversion blendfile is open, a permanent warning is shown in the footer of the UI. > * By clicking on it, user can get more info. This doesn't seem to be implemented? I don't see code to handle this.
@ -29,3 +29,3 @@
# Stats & Info
row.label(text=context.screen.statusbar_info(), translate=False)
layout.template_status_info()
Member

Rather than just moving this all to an ad-hoc template (which kind of defeats the purpose of templates), I'd propose something like this:

if context.blend_data.has_compatibility_issues:
  layout.template_compatibility_issues()
else:
  row.label(text=context.screen.statusbar_info(), translate=False)
Rather than just moving this all to an ad-hoc template (which kind of defeats the purpose of templates), I'd propose something like this: ```py if context.blend_data.has_compatibility_issues: layout.template_compatibility_issues() else: row.label(text=context.screen.statusbar_info(), translate=False) ```
Author
Owner

I would rather not. I do think the whole status info area does deserve a template - this warning about forward compatibility is only the initial complex UI change here, there are potentially many more (missing linked data? missing external files? etc.). not to mention also the rest of the status info could use some love.

Find it way more logical and coherent as a single template

I would rather not. I do think the whole status info area does deserve a template - this warning about forward compatibility is only the initial complex UI change here, there are potentially many more (missing linked data? missing external files? etc.). not to mention also the rest of the status info could use some love. Find it way more logical and coherent as a single template
@ -51,0 +51,4 @@
/**
* Check whether a given path is actually a readable .blend file.
*/
bool BKE_blendfile_is_readable(const char *path, struct ReportList *reports);
Member

The name & comment bother me a bit. This looks like a simple query, maybe checking if the path exists. But it actually opens the file and collects reports from that. So it's more of a BKE_blendfile_attempt_to_open_and_close(). A bit of an awkward name though, at least the comment should be more clear.

This may not actually be needed though (see below regarding library hint drawing).

The name & comment bother me a bit. This looks like a simple query, maybe checking if the path exists. But it actually opens the file and collects reports from that. So it's more of a `BKE_blendfile_attempt_to_open_and_close()`. A bit of an awkward name though, at least the comment should be more clear. This may not actually be needed though (see below regarding library hint drawing).
mont29 marked this conversation as resolved
@ -1081,0 +1087,4 @@
{
BLI_assert(fd->filesdna != nullptr);
for (BHead *bhead = blo_bhead_first(fd); bhead; bhead = blo_bhead_next(fd, bhead)) {
if (bhead->code == BLO_CODE_GLOB) {
Member

Yikes @ this big if-block - makes me scroll back and forth to check the scope :) Could use continue here?

Yikes @ this big if-block - makes me scroll back and forth to check the scope :) Could use `continue` here?
mont29 marked this conversation as resolved
@ -6510,0 +6533,4 @@
uiBut *but;
const uiStyle *style = UI_style_get();
uiLayout *ui_abs = uiLayoutAbsolute(layout, false);
Member

Yikes... But I see that's what regular reports do too... :/

Yikes... But I see that's what regular reports do too... :/
mont29 marked this conversation as resolved
@ -6510,0 +6538,4 @@
eUIEmbossType previous_emboss = UI_block_emboss_get(block);
UI_fontstyle_set(&style->widgetlabel);
int width = static_cast<int>(
Member

Use functional style cast (int width = int(...)) https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast

Use functional style cast (`int width = int(...)`) https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
mont29 marked this conversation as resolved
@ -6510,0 +6540,4 @@
UI_fontstyle_set(&style->widgetlabel);
int width = static_cast<int>(
BLF_width(style->widgetlabel.uifont_id, status_info_txt, strlen(status_info_txt)));
width = max_ii(width, static_cast<int>(10 * UI_SCALE_FAC));
Member

Functional style cast.

Functional style cast.
mont29 marked this conversation as resolved
@ -6510,0 +6597,4 @@
UI_BTYPE_BUT,
0,
ICON_ERROR,
static_cast<int>(3 * UI_SCALE_FAC),
Member

Functional style cast.

Functional style cast.
mont29 marked this conversation as resolved
@ -6510,0 +6617,4 @@
status_info_txt,
UI_UNIT_X,
0,
static_cast<short>(width + UI_UNIT_X),
Member

Functional style cast.

Functional style cast.
mont29 marked this conversation as resolved
@ -31,6 +31,7 @@
#include "DNA_userdef_types.h"
#include "DNA_workspace_types.h"
#include "BKE_blender_version.h"
Member

Is this needed?

Is this needed?
Author
Owner

not anymore indeed

not anymore indeed
mont29 marked this conversation as resolved
@ -1245,0 +1295,4 @@
const bool is_asset_browser = ED_fileselect_is_asset_browser(sfile);
const bool is_library_browser = !is_asset_browser &&
filelist_islibrary(sfile->files, blendfile_path, nullptr);
if (!is_asset_browser && !is_library_browser) {
Member

Would remove this since the conditions are checked below anyway (redundant), and it has to be modified as new cases get added (which isn't clear while appending at the bottom of the function).

Would remove this since the conditions are checked below anyway (redundant), and it has to be modified as new cases get added (which isn't clear while appending at the bottom of the function).
mont29 marked this conversation as resolved
@ -1255,0 +1324,4 @@
if (!sfile->runtime->is_blendfile_status_set) {
BKE_reports_clear(&sfile->runtime->is_blendfile_readable_reports);
sfile->runtime->is_blendfile_readable = BKE_blendfile_is_readable(
Member

This could all be part of the file-list loading:

  • When loading fails this can be saved in the file-list, e.g. when BLO_blendhandle_from_file() returns null in filelist_readjob_list_lib(), a FL_HAS_INVALID_LIBRARY file-list flag can be set.
  • Reports from it can also be stored in FileList rather than being ignored (RPT_STORE must be set!).
  • Then we can just check for is_library_browser and the FL_HAS_INVALID_LIBRARY flag here, and draw the hint with the reports in the file-list. (We won't draw a hint for recursive loading, even if the file-list has the "has invalid library" flag set, which seems like the wanted behavior.)
  • BKE_blendfile_is_readable() isn't needed then.

I'm not feeling too strongly about this though. It seems sensible to do this way and avoids hacking this in with the runtime struct. But may be more complicated than worth it. I admit it's also nice to keep things together in this rather small scope.

This could all be part of the file-list loading: - When loading fails this can be saved in the file-list, e.g. when `BLO_blendhandle_from_file()` returns null in `filelist_readjob_list_lib()`, a `FL_HAS_INVALID_LIBRARY` file-list flag can be set. - Reports from it can also be stored in `FileList` rather than being ignored (`RPT_STORE` must be set!). - Then we can just check for `is_library_browser` and the `FL_HAS_INVALID_LIBRARY` flag here, and draw the hint with the reports in the file-list. (We won't draw a hint for recursive loading, even if the file-list has the "has invalid library" flag set, which seems like the wanted behavior.) - `BKE_blendfile_is_readable()` isn't needed then. ---- I'm not feeling too strongly about this though. It seems sensible to do this way and avoids hacking this in with the runtime struct. But may be more complicated than worth it. I admit it's also nice to keep things together in this rather small scope.
Author
Owner

i would rather keep that for a potential future improvement. Also keep in mind that this patch also needs to land in current LTS releases, so trying to keep it as limited as possible is important. It's already waaaaaayyy to big to my liking tbh :(

i would rather keep that for a potential future improvement. Also keep in mind that this patch also needs to land in current LTS releases, so trying to keep it as limited as possible is important. It's already waaaaaayyy to big to my liking tbh :(
@ -1961,6 +1961,11 @@ BlendHandle *filelist_lib(FileList *filelist)
return filelist->libfiledata;
}
int filelist_files_num_entries(FileList *filelist)
Member

Better make it clear in the name that this includes filtered out items. If I wanted to have the count of visible items, I'd just choose this function if it pops up in auto-completion (an actual bug).

Better make it clear in the name that this includes filtered out items. If I wanted to have the count of visible items, I'd just choose this function if it pops up in auto-completion (an actual bug).
Author
Owner

I would expect such a function returning the amount of filtered entries to be called filelist_files_num_entries_filtered or something like that... This would also match the internal data (entries_num vs. entries_filtered_num).

Will add comment, but imho name itself is fine.

I would expect such a function returning the amount of filtered entries to be called `filelist_files_num_entries_filtered` or something like that... This would also match the internal data (`entries_num` vs. `entries_filtered_num`). Will add comment, but imho name itself is fine.
mont29 marked this conversation as resolved
@ -3764,3 +3977,1 @@
if (WM_operator_name_call(C, "WM_OT_save_mainfile", WM_OP_EXEC_DEFAULT, nullptr, nullptr) &
OPERATOR_CANCELLED)
{
if (bmain->has_forward_compatibility_issues) {
Member

It's not clear why this case is different, better explain in a comment (why does it use invoke and why does it always execute the callback, even when the operator is cancelled?).

It's not clear why this case is different, better explain in a comment (why does it use invoke and why does it always execute the callback, even when the operator is cancelled?).
mont29 marked this conversation as resolved
@ -3814,0 +4034,4 @@
UI_BTYPE_BUT,
0,
0,
has_forwardcompat_issues ? IFACE_("Save As...") : IFACE_("Save"),
Member

Could also use a comment as to why has_forwardcompat_issues would change the displayed text.

Could also use a comment as to why `has_forwardcompat_issues` would change the displayed text.
mont29 marked this conversation as resolved
Hans Goudey changed title from Implementation of new blendfile compatibility policy. to Implementation of new blendfile compatibility policy 2023-07-26 17:58:31 +02:00
Member

UI wise things look good, although the PR description could use some screenshots. Here are some.

On Ctrl+S with a file from a newer Blender version:

Opening a new file while the current file from a newer Blender version contains unsaved changes:

Open a file supposedly saved in 5.0:

Attempting to link/append from a 5.0 file:

UI wise things look good, although the PR description could use some screenshots. Here are some. On `Ctrl+S` with a file from a newer Blender version: <img src="/attachments/a25e3d8a-f2ca-4a65-b67a-1936a8321d36" width="600px"/> Opening a new file while the current file from a newer Blender version contains unsaved changes: <img src="/attachments/3fadb01a-eb4e-4619-a5fb-7a8222808fa5" width="600px"/> Open a file supposedly saved in 5.0: <img src="/attachments/483d7dc0-d141-47af-9b2e-14130c25b376" width="600px"/> Attempting to link/append from a 5.0 file: <img src="/attachments/005d0608-f9f8-4200-89f8-6bed82e02782" width="600px"/>
Author
Owner
  • When a newer fileversion blendfile is open, a permanent warning is shown in the footer of the UI.
    • By clicking on it, user can get more info.

This doesn't seem to be implemented? I don't see code to handle this.

This was in initial implementation, but later on in that review it was decided to go back to a simple tooltip message for now.

> > * When a newer fileversion blendfile is open, a permanent warning is shown in the footer of the UI. > > * By clicking on it, user can get more info. > > This doesn't seem to be implemented? I don't see code to handle this. This was in initial implementation, but later on in that review it was decided to go back to a simple tooltip message for now.
Bastien Montagne added 2 commits 2023-07-26 19:49:31 +02:00
Bastien Montagne requested review from Julian Eisel 2023-07-26 19:49:32 +02:00
Julian Eisel approved these changes 2023-07-27 10:58:41 +02:00
Author
Owner

Committed as the commits listed above, closing.

Committed as the commits listed above, closing.
Bastien Montagne closed this pull request 2023-07-27 16:36:05 +02:00

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110109
No description provided.