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

Control and form colors appear to randomly change during export/build #183

Closed
hecon5 opened this issue Feb 26, 2021 · 90 comments
Closed

Control and form colors appear to randomly change during export/build #183

hecon5 opened this issue Feb 26, 2021 · 90 comments

Comments

@hecon5
Copy link
Contributor

hecon5 commented Feb 26, 2021

I've noticed that the exported colors appear to randomly change, even if they appear correctly in the form. This seems to be especially prevalent when using themes to map colors.

@hecon5 hecon5 mentioned this issue Mar 5, 2021
@hecon5
Copy link
Contributor Author

hecon5 commented Mar 5, 2021

I should clarify: I mean the exported code color changes; I do not notice any change in the actual displayed color on the Form/Reports.

@hecon5 hecon5 closed this as completed Mar 5, 2021
@hecon5
Copy link
Contributor Author

hecon5 commented Mar 5, 2021

I commented on and closed the wrong issue...gah.

@hecon5 hecon5 reopened this Mar 5, 2021
@A9G-Data-Droid
Copy link
Contributor

A9G-Data-Droid commented Mar 5, 2021

Here is a discussion that offers an explanation:
https://stackoverflow.com/questions/4608167/color-constants-in-access-2007

Due to the fact that these colors are references to system and thus dynamic I would close this as CANTFIX\WONTFIX

@joyfullservice
Copy link
Owner

Looking at the source behind a form control, it appears that the BorderColor property contains everything Access needs to know about the color...

image

Perhaps it tries to match it against the theme color schemes and shade variants first, and if it matches, it shows the theme color, but if no match is found, it just displays the actual color...

@hecon5
Copy link
Contributor Author

hecon5 commented Mar 5, 2021

The BackColor does this, too. An example (button) with a theme:
I'm actually not 100% certain how this works, it appears that the color is used, if the theme isn't present; the color just happens to be the latest that was saved / exported.
image

@joyfullservice
Copy link
Owner

Thank you all for the good input on this! I think I will go ahead and close out this issue since it does not appear to be practically feasible to standardize these values across different systems.

@joyfullservice
Copy link
Owner

I came across a clue today that gives me a glimmer of hope that we may actually be able to solve this! It is not a trivial change, but my hunch is that we could ultimate resolve this problem through the sanitize function/class.

Here is the gist of the idea. If you look at the examples above, or other examples in your own projects, you will see that on the controls where the color values are changing, they are also using a theme color index, tint and shade.

image

My hunch is that when theme color index is used, the color value is simply representative, based on the graphics/monitor setup on that computer. The first time the form is opened, the actual color is calibrated for the system and updated. That's why you will see this value change between exports, depending on whether or not you have opened the form since importing it. Perhaps the color (placeholder?) exists just in case the theme definition is not found, so Access can at least somewhere close in representing the color. (That part is pure conjecture.)

When I compare this to other color values that don't change between exports, I find that the colors that don't change are the ones that are using absolute color values, not theme index/tint/shade. For these, the theme index is set to -1.

image

My hypothesis is that we could take one of the two following approaches to standardize the export files and resolve this issue:

  1. Discard the placeholder color value, since this will be dynamically generated automatically. If this doesn't create any errors on import, and doesn't cause any changes to the color, this would be the preferred option.
  2. If the above doesn't work, a second approach would be to compute the placeholder value ourselves in a way that is generically consistent between systems. This would be more involved because it would mean extracting and parsing the theme xml file to determine the actual color values of the theme index, and factor in the tint and shade, but is still technically possible.

With either approach we will need to use a more sophisticated parsing function because we need to link these related lines to determine what to do with the color value line. (Keep it, discard it, or compute it ourselves.) We would need to read all the property lines for the control before we could make the determination, and multiple properties of the control involve color values. It would probably need a two-pass approach where we read all the lines first, then make any adjustments before writing the finalized control block section.

While this is obviously not a quick and simple fix, it does seem like there is a good possibility that this will allow us to ultimately resolve the issue with unwanted change noise appearing in source files between builds, caused by theme-based automatically generated color values.

If anyone has opportunity to do some more research/testing/development on this idea, please post your findings here! 😄

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 3, 2021

It's funny, I was raging at Access's color functions today, too; for a similar issue: Themes.

I posted it on this SO Theme question , if you have any ideas, but for now I like your thoughts.

FWIW, I don't think option 1 will work, because I've noticed with custom themes set, I have to close and then reopen the Access file for the theme to be "respected". Once I do this, it works flawlessly. That said...you might be onto something. I'll do some quick and dirty testing and see what happens.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 4, 2021

Update: it turns out... Option 1 might actually work!

I deleted a bunch of color definitions from fields, and it imported fine! This ... could be very promising!

Now to determine how best to do the sanitize (should this be integral to clsDbForm or part of the modSanitize? My thoughts are that probably best in modSanitize, though you raise an interesting idea for a whole sanitize class, cleaner code, but implementation would be more difficult.

@A9G-Data-Droid
Copy link
Contributor

I agree. I think option 1 is best. Unfortunately I'm not seeing a clear pattern that relates the name of the theme and the associated color definitions. We might have to build a dictionary that maps them so that dictionary can be looped. Ugh.

@joyfullservice
Copy link
Owner

@hecon5 - Thanks for the update! That does sound very encouraging!

After thinking through the implementation details this morning, I think you are right that modSanitize is probably the best place to implement this, inline with the other sanitizing code. Unfortunately it does add some complexity to the function, since we can't just loop through the lines and make the keep or discard decision on the first pass, like we do now.

Since performance is very crucial in this function, I am thinking that the best way to do this is to loop through all the lines like we currently do, but instead of building the new file contents at the same time, we would instead be building an array of line numbers that we want to exclude from the new file. The first pass would involve all the logic of making those decisions of what to include and what to discard.

We would then sort the array of lines to exclude so that on the second pass through the lines of the file we could also incrementally iterate through the lines to exclude as we build the actual file contents using the concatenation class. This allows us to use very lightweight numerical comparisons with the array and avoid unnecessary loops. Taking this approach, I think the performance would be very similar to the current highly optimized function.

Adding to the complexity here is that I believe control definition blocks can be nested, requiring us to use a call-stack type of concept to ensure that we are linking the line of the *ColorIndex to the *Color of the same control. (Since the two might be on either side of another nested definition block) This should be manageable using nested dictionaries relative to the indent position in the file. When we reach the end of a section definition, we would then make the determination on which *Color lines should be discarded, and remove the dictionary that represents that section.

@joyfullservice
Copy link
Owner

I agree. I think option 1 is best. Unfortunately I'm not seeing a clear pattern that relates the name of the theme and the associated color definitions. We might have to build a dictionary that maps them so that dictionary can be looped. Ugh.

Would this even matter if we are taking option 1 and simply discarding the lines with the color definitions? Or maybe I am missing something here... 🤔

@A9G-Data-Droid
Copy link
Contributor

A9G-Data-Droid commented Jun 4, 2021

@joyfullservice are you talking about always dropping color definitions? Or only dropping definitions when the associated theme index is <> -1 ? Maybe I misunderstood.

@joyfullservice
Copy link
Owner

@joyfullservice are you talking about always dropping color definitions? Or only dropping definitions when the associated theme index is <> -1 ? Maybe I misunderstood.

Correct. I am looking at only dropping them if the *ColorIndex is <> -1. (As shown in the top example of the image I posted yesterday.)

@A9G-Data-Droid
Copy link
Contributor

So you need to first establish the relationship between the related entries. Maybe this could be done with an access table? Then for each index that is <> -1 remove related lines. If the lines were split on the = character then you could do a quick lookup on the LHS of the = to get the line number. Then remove those lines from the line collection.

@joyfullservice
Copy link
Owner

joyfullservice commented Jun 4, 2021

@A9G-Data-Droid - Yes, that's the general idea. Establish the relationship between the lines, and remove the color line if the ThemeColorIndex is <> -1. I am thinking that a dictionary is going to be more performant than a table, especially since it has the Exists function to quickly locate a key.

For example, if I was looping through the lines, and found ForeColor =12673797, I would add this to a dictionary object with ForeColor as the key, and the line number as the value. Then later in the same section, if I found ForeThemeColorIndex =10 then I would know that I can discard the ForeColor line. If I found the first dictionary entry, I would add this line number as a line to exclude in the final output file.

For reference, it looks like the following list would be the complete set of color index properties we are looking for, based on the Access 2010 object model.

BackThemeColorIndex
AlternateBackThemeColorIndex
BorderThemeColorIndex
ForeThemeColorIndex
GridlineThemeColorIndex
HoverForeThemeColorIndex
HoverThemeColorIndex
PressedForeThemeColorIndex
PressedThemeColorIndex

joyfullservice added a commit that referenced this issue Jun 4, 2021
This prepares the way to do more involved sanitizing such as removing dynamically generated color values. See #183
joyfullservice added a commit that referenced this issue Jun 4, 2021
Initial implementation of process discussed in #183
@hecon5
Copy link
Contributor Author

hecon5 commented Jun 4, 2021

Two things: It appears it's not just "me" with the off by one issue, -1 is technically undefined, and msoNotThemeColor = 0. Whatever. I digress; that's cost me enough headache this week.

I also have these in some of my forms:

 DatasheetBackThemeColorIndex
 DatasheetGridlinesColorThemeColorIndex

Which makes me wonder, instead of trying to ensure we have a complete list, would it be more accurate to match against "ThemeColorIndex" and then see if the right of that string is " =-1" or not?

This would ensure we don't need to know all the possibilities (seems like we're still missing some).

@A9G-Data-Droid
Copy link
Contributor

Yeah you could write a generic <= 0 based test but which lines would you omit for each ThemeColorIndex?

Would you just remove "ThemeColorIndex" and wildcard match against the prefix? I guess we would have to see if that pattern is consistent.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 4, 2021

I'm not sure we want to do a generic <=0 test...I see there's a msoThemeColorMixed "-2", which leads me to think that the msoNotThemeColor (-1) should probably be a constant we can change if and when MSFT ever fixes this oddity; and that there's a chance that whatever msoThemeColorMixed is might mean we don't want to remove those colors.

@joyfullservice
Copy link
Owner

What I am seeing is that you could probably replace ThemeColorIndex with Color to set the base color. I like that idea of simplifying the comparison, but I did notice one property that seemed inconsistent with the others:

image

@hecon5 - Were you indicating that this was coming through as DatasheetGridlinesColorThemeColorIndex on your system?

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 4, 2021

I'm not sure we want to do a generic <=0 test...I see there's a msoThemeColorMixed "-2", which leads me to think that the msoNotThemeColor (-1) should probably be a constant we can change if and when MSFT ever fixes this oddity; and that there's a chance that whatever msoThemeColorMixed is might mean we don't want to remove those colors.

In terms of what we should remove (red is removal, green is "kept").

' For values that have a theme:  ([Border] = [ColorName] Below)
-- BorderColor =16777215
++ BorderThemeColorIndex =2
++ BorderTint =60.0

' For controls/components that don't use a theme: ([HoverFore] = [ColorName] Below)
++  HoverForeColor =0
++  HoverForeThemeColorIndex =-1

More generically, for a given [ColorName] [Color], delete the matching [Color] line where the [ColorName] [ThemeColorIndex] =-1

@joyfullservice yes, on a few forms; not often, but it's there, and if I change it, it does affect the form look.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 4, 2021

What I see is:

   DatasheetGridlinesColor =15132391
 
    <Buncha unrelated field info like you have>

    DatasheetAlternateBackColor =15921906
    DatasheetGridlinesColor12 =0
    FitToScreen =1
    DatasheetBackThemeColorIndex =1
    BorderThemeColorIndex =3
    ThemeFontIndex =1
    <more stuff>

joyfullservice added a commit that referenced this issue Jun 8, 2021
Using a dedicated option for this, especially since it may involve doing a special repair on the host database. See
#183 and #235.
@A9G-Data-Droid
Copy link
Contributor

I am unable to build the dev branch using the 3.3.17 or 3.4.0 versions I have.

@joyfullservice
Copy link
Owner

I am unable to build the dev branch using the 3.3.17 or 3.4.0 versions I have.

You should be able to build it from 3.4.1.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

More thoughts I've been cogitating on today and last night while sitting in waiting areas: We should have "levels" of color sanitize aggressiveness, (off / robust / aggressive). This could allow us to give most users ability to tune to their desired "noise" level.

I think we've got good evidence themed controls sanitize properly (that part seems to be consistent and robust), as do the ones where the theme is set to -1, but less so where the theme data isn't present at all.

It would also allow us to test and improve the helper routine as well as sanitizing for the edge cases ("Access laziness") and then if we determine we're able to detect and remove hard set values where the theme insert didn't work properly, we can do so then.

I hate code noise as a developer. Especially in Git, and VBA has a tooooon of these issues. If we can remove even 10% of color noise, it'll make a huge difference to me. It makes me panic a little bit every time I see a lot of forms exported that didn't get work done, and adds work to my workflow. Annoying.

BUT I'm dead set against features we aren't sure are going to work upon rebuild. The most annoying bugs to troubleshoot for users are ones on forms/code I didn't change in [insert long time here]. I have a few persnickety users who notice (and "ask" me) whenever colors change slightly, and it's annoying to figure out what happened (history, it's been mostly my own fault upon rebuild). I really really really do not want to deal with this addin helping me there.

That said, and "in between " option would make my life so much better. It's reliable (for themed controls, I didn't notice any issues at all). It would also encourage me to use themes for new content, and eventually go back and set themes for old components (which also has the upside of making UI color changing a LOT easier when the powers that be decide to change the app color requirements...again.).

I like the idea of a "repair" helper function, but it's too "new" to run on some of the larger tools I have right now. But I also very much want to cut out noise on exports for 90% of my forms. Which is why I'd like something in between all or nothing with colors.

So, with all that, I am in favor of a 3 level option, strongly.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

Added PR #238 to start fleshing this out.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

image

Mocked up how this might work.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

image

Now maps correctly to the sanitize level setting. :)

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 9, 2021

Ok, testing this on some dbs that lost colors previously; let's see what happens!

Initial export using eSanitizeLevel of eslRobust led to most colors where themed being correctly removed. Rebuild looks ok at first glance.

Initial export using eSanitizeLevel of eslToTheBoneBeta led to the extra fields being removed that caused me issues before.

If we add the helper routine @joyfullservice added, perhaps if eslAgressiveor eslToTheBoneBeta is chosen a button is exposed to allow you to click it and 'repair' your controls?

@joyfullservice
Copy link
Owner

@hecon5 - Thanks for your work on fleshing this out! Just to clarify, are there problems (VCS noise) that eslToTheBoneBeta fixed that were not fixed by eslAgressive? In other words, is there a real use-case for the most extreme level of sanitizing color values?

joyfullservice added a commit that referenced this issue Jun 10, 2021
Based on discussion in #183, it may be too aggressive to assume that most controls use theme colors unless otherwise specified. Backing off the sanitizing of these colors until real-world testing confirms whether this is actually necessary.
@hecon5
Copy link
Contributor Author

hecon5 commented Jun 10, 2021

I think, for me, I have way too many controls that are stripped bare and don't have colors (ported from legacy versions) that use hand-set colors that don't rebuild correctly.

I think some users may benefit from it, and if all my controls were changed to themed controls (eventually), then the to the bone level would be nice. For now, it's not always predictable as to what's not going to work with the sanitize routines I set to 'esltothebonebeta'.

If we can figure that out, we could set those lower, but for now, they are way too aggressive for a reliable rebuild in my environment.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 10, 2021

In any regard, I think having levels would allow us to also tune things and introduce features that can be vetted on small databases first, or big ones, as you could just see what changes with the export without redeploying a new addin during testing.

@A9G-Data-Droid
Copy link
Contributor

Are custom themes supported? Currently, my custom theme is not being restored or set. I have to set my theme after each rebuild to get my colors back.

@joyfullservice
Copy link
Owner

Are custom themes supported? Currently, my custom theme is not being restored or set. I have to set my theme after each rebuild to get my colors back.

Yes, custom themes should be supported. Go ahead and open a new issue for this with some details, and let's work to get that fixed. 😄

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 17, 2021

With the levels implemented, and the error prone sanitization routines in the beta level, theme colors import properly, as do manually set colors. I'm going to do some more testing, but I think this can be closed.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 17, 2021

Nevermind, still having some odd behavior where themes are sometimes not set correctly; troubleshooting now.

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 17, 2021

Full round trip on my more complex database appears to work!

@hecon5
Copy link
Contributor Author

hecon5 commented Jun 17, 2021

@A9G-Data-Droid, if your testing comes up good, I think we're ready to close this.

@A9G-Data-Droid
Copy link
Contributor

I'm seeing some random color change noise using the dev branch version 3.4.5

I have the Sanitize Colors set to Aggressive

I see these ones changing on most forms:

image

@joyfullservice
Copy link
Owner

Would you be able to expand that to show the whole block? (For example, I am wondering if the PressedThemeColorIndex property is shown below the screenshot.)

You could also try the Advanced (Beta) option to see how your database handles it. (That assumes more defaults to accommodate incomplete block structures.) At present, the only difference between this and Aggressive is that Advanced (Beta) removes color values when the ThemeColorIndex property is not found.

The following code snippet comes from modSanitize:

    ' Loop through properties, checking for index
    For intCnt = 0 To UBound(varBase)
        strKey = varBase(intCnt) & "ThemeColorIndex"
        If dBlock.Exists(strKey) Then
            If dBlock(strKey) <> NO_THEME_INDEX Then
                ' Check for corresponding color property
                strKey = varBase(intCnt) & "Color"
                If dBlock.Exists(strKey) Then
                    ' Skip the dynamic color line
                    SkipLine dBlock(strKey)
                End If
            End If
        Else
            Select Case dBlock("Type")
                Case "Section", "FormHeader", "FormFooter"
                    ' Some controls like form sections don't use color values
                    ' if a theme index is specified. If a color value exists,
                    ' we should preserve it.
                Case Else
                    ' Most controls automatically use theme indexes
                    ' unless otherwise specified.
                    ' As discussed in #183, this can be affected by incomplete
                    ' component definition blocks.
                    If Options.SanitizeColors = eslAdvancedBeta Then
                        strKey = varBase(intCnt) & "Color"
                        If dBlock.Exists(strKey) Then
                            ' Skip the dynamic color line
                            SkipLine dBlock(strKey)
                        End If
                    End If
            End Select
        End If
    Next intCnt

@joyfullservice
Copy link
Owner

@A9G-Data-Droid - Is this working on the latest build (3.4.13) from the dev branch? Between the sanitize updates and initializing forms I think this might be resolved now. Would you be able to verify this so we can close out this issue?

@joyfullservice joyfullservice added the pending resolved Possibly resolved, needs testing or confirmation label Jul 14, 2021
@A9G-Data-Droid
Copy link
Contributor

Ok, I did the full round trip on my main project DB. I'm seeing the same color noise as before (BackColor, HoverColor, and PressedColor). Honestly, it's not a show stopper. I wouldn't let this issue hold up 3.4 release.

When I run with "Advanced (Beta)" it does remove a few color lines, including the noisy ones. This doesn't seem to be destructive with my project. The built and rebuilt versions are stable. Colors look correct. When I use this advanced sanitation profile this issue is completely resolved.

@hecon5
Copy link
Contributor Author

hecon5 commented Jul 15, 2021

Huzzah!

@hecon5 hecon5 closed this as completed Jul 15, 2021
@joyfullservice
Copy link
Owner

Awesome! From the best that I understand, the "Advanced (Beta)" seems to be needed in limited cases where the form block structures are not fully complete. Thanks again for letting us know how it went on your project database!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants