Excel VBA bug/anomaly - ActiveWorkbook.Save changes Workbook_BeforeSave function

Question

In my code I use a Workbook_BeforeSave function that does some text formatting.

When I hit the Save button, it runs and formats the size and font type of some cells.
Here is part of my code that does the job:

Private Sub Workbook_BeforeSave(ByVal SaveAsUI As Boolean, Cancel As Boolean)

Dim c As Range
Dim rng As Range
Set rng = ActiveSheet.UsedRange.Cells

For Each c In rng
    If ispcname(c.Value) = True Or isip(c.Value) = True Then ActiveSheet.Hyperlinks.Add Anchor:=c, Address:="": c.HorizontalAlignment = xlCenter: c = StrConv(c, vbProperCase): c.Font.Name = "Arial": c.Font.Size = "10"
    If Right(c, 1) = "$" Then 
        y = c.Column: x = c.Row

        Dim i As Integer
        For i = 1 To rng.Rows.Count
            If LCase(Cells(i, y).Value) = "backup" Then
                If  Right(c, 1) = "$" Then Cells(x, y) = Cells(x, y - 2) & "$": ActiveSheet.Hyperlinks.Add Anchor:=c, Address:="": c.Font.Name = "Calibri": c.Font.Size = "10": c.HorizontalAlignment = xlCenter: c.Font.Color = RGB(192, 0, 0)
            End If
        Next i
    End If
Next c
End Sub

I have recently implemented a code that will save the Workbook if it is closed.

Private Sub Workbook_BeforeClose(Cancel As Boolean)
    Application.DisplayAlerts = False
        ActiveWorkbook.Save
    Application.DisplayAlerts = True
End Sub

Then something went wrong that I can't explain. When ActiveWorkbook.Save runs, the cells that should change into Calibri, change into Arial instead, size remains unmodified, color is working normally. However when I manually hit the save button it works as it should. (changes the cells back to Calibri)

There is no other code interfering because when I commented out the part that changes the font type to Calibri, the ActiveWorkbook.Save also stopped changing it into Arial as well.

My questions are:

Why is this happening? Is this a bug?
Is there any workaround?

I am using Excel 2007.

Could the value of `ispcname(c.Value) = True Or isip(c.Value) = True` be affecting it? Also, maybe use `ThisWorkbook.Save` or even `ActiveSheet.Parent.Save` instead of `ActiveWorkbook`. I'd also split the `If isbackup = True And Right(c, 1) = "$" Then Cells(x, y) = Cells(x, y....` line into seperate rows within an `IF...END IF` block rather than `:` separated commands (easier to read). — Darren Bartrup-Cook, Dec 01 '16 at 15:27
`ispcname(c.Value) = True Or isip(c.Value) = True` functions only checks some conditions with the value and returns a Boolean, shouldn't be the reason (I will test it anyway). Tried `ActiveSheet.Parent.Save` same result as `ThisWorkbook.Save`. I also taught of separating it for better performance. — Divin3, Dec 01 '16 at 15:35

score 1 · Accepted Answer · edited Sep 30 '17 at 17:27

Not sure why this is happening at all, but one workaround seems to be calling Workbook_BeforeSave manually and then disabling it for the ActiveWorkbook.Save call:

Private Sub Workbook_BeforeClose(Cancel As Boolean)
    Application.DisplayAlerts = False
    Application.EnableEvents = False
    Workbook_BeforeSave False, False    'Manual call.
    Me.Save                             'Save without the event firing again.
    Application.EnableEvents = True
    Application.DisplayAlerts = True
End Sub

That said, you also have some curious logic in the Workbook_BeforeSave handler. First, I don't think that For i = 1 To rng.Rows.Count is doing what you think it's doing. It isn't necessarily going to loop over the entire column, because UsedRange.Cells doesn't have to start at row 1. If your used range is something like $A$4:$Z$100, rng.Rows.Count will be 97 and all of your references to Cells(i, y) would be off by 3.

It also isn't clear if ispcname(c.Value) = True Or isip(c.Value) = True and Right(c, 1) = "$" are mutually exclusive. If they are, If Right(c, 1) = "$" should actually be an ElseIf.

Couple other things:

Executing 5 different statements in a one line If statement is incredibly hard to read and that makes it error prone. Use actual If...End If blocks unless the action is something trivial like Exit Sub.
The second If Right(c, 1) = "$" Then is always true. It can be removed completely.
After actually formatting your code in a readable way, it's clear that you are using properties of c all over the place inside the For Each c In rng loop. I'd put it in a With block.
You only need to use ActiveSheet once. After that, you can either get it from rng.Parent or (much better) get a reference to it.
Get in the habit of using String returning functions instead of Variant returning functions when you need a String. I.e. Right$ instead of Right - the latter performs an implicit cast.
Fully qualify all of your references to Cells.
Avoid implicitly using the default properties of objects, i.e. Range.Value.
Use Long for row counters, not Integer to avoid the possibility for overflows.
Use vbNullString instead of the literal "".
Font.Size is measured in points. It should be a number, not a string.

It should look more like this:

Private Sub Workbook_BeforeSave(ByVal SaveAsUI As Boolean, Cancel As Boolean)
    Dim c As Range
    Dim sh As Worksheet
    Set sh = ActiveSheet    'Tip 4
    Dim rng As Range
    Set rng = sh.UsedRange.Cells

    For Each c In rng
        With c  'Tip 3
            If ispcname(.Value) Or isip(.Value) Then  'Tip 1
                sh.Hyperlinks.Add Anchor:=c, Address:=vbNullString    'Tips 4 and 9
                .HorizontalAlignment = xlCenter
                .Value = StrConv(.Value, vbProperCase)  'Tip 7
                .Font.Name = "Arial"
                .Font.Size = 10     'Tip 10
            End If  'Pretty sure this should be an ElseIf structure here.
            If Right$(.Value, 1) = "$" Then  'Tips 5 and 7.
                y = .Column
                x = .Row
                Dim i As Long   'Tip 8
                For i = 1 To rng.Rows.Count     'This is most likely wrong.
                    'Tip 2 used to be here.
                    If LCase$(sh.Cells(i, y).Value) = "backup" Then     'Tips 1, 5, and 6
                        .Value = sh.Cells(x, y - 2).Value & "$"   'Tips 4, 6, and 7
                        sh.Hyperlinks.Add Anchor:=c, Address:=vbNullString    'Tips 4 and 9
                        .Font.Name = "Calibri"
                        .Font.Size = 10     'Tip 10
                        .HorizontalAlignment = xlCenter
                        .Font.Color = RGB(192, 0, 0)
                    End If
                Next i
            End If
        End With
    Next c
End Sub

Really nice answer. The workaround part works as a charm. I know this is not the best coding. I wrote this part when I was still beginner to code VBA. That `For i = 1 To rng.Rows.Count` is to find a cell thats value is "backup" if found, under it if it has any cell that ends in character "$", copy the cell left from it by 2, and add the "$". The cells in the left are PC names that are used to connect to them by remotely, and in the right are hyperlinks to their backup. There is a `SheetFollowHyperlink` function that detects if it is a pc name -> open remotely, if pcname+"$", open its backup — Divin3, Dec 01 '16 at 18:16
1. You are right, I will implement it. 2. "$" could be any random character, I use it to separate it from the PC names in the `SheetFollowHyperlink` function. 3. In newly written parts of my code, I use the `with` block. You are right, I will rewrite this as well. 4. True. 5. I don't understand this one. 6. Nor this point. 7. Got it. 8. Also a good idea but these lists are meant to be short like 100 x 20 maximum. 9. Have to do some research about this. 10. Back in the days this is how I found it on the web. Also useful info. I'll start working on implementing your code. And give a feedback. — Divin3, Dec 01 '16 at 18:26
@Divin3 - Glad it worked. The point of the note about `rng.Rows.Count` is the possibility that your indexing will get messed up if you have empty rows at the top of the sheet. If you need to test the entire column, it's much more robust. `c.Row` is the absolute row number relative to the sheet, but `i` is relative to the first of `UsedRange`. — Comintern, Dec 01 '16 at 18:31
@Divin3 - Added hyperlinks on some of the items you said you didn't understand. — Comintern, Dec 01 '16 at 18:37
`.Value = StrConv$(.Value, vbProperCase)` gives me a "Sub of function not defined" error if I use `$`. The rest is working perfectly. — Divin3, Dec 01 '16 at 19:06
@Divin3 - You're right. Not sure why I was thinking that `StrConv` had a `Variant` overload. Corrected. — Comintern, Dec 01 '16 at 19:08

D. O. · Answer 2 · 2016-12-01T16:27:59.100

0

If you are using "$" to say that the cell is a currency, don't test if the last character is "$". you have to check if the cell has a currency format.

Modify the line

if right(c,1)= "$"

to

fc = c.NumberFormat
    If InStr(1, c, "$") = 0 Then ...

your test will never find the "$" in c.

edited Dec 01 '16 at 16:27

answered Dec 01 '16 at 15:35

D. O.

616
1
11
25

1

if there is a `Then` in the same line as the `If`, there is no need for `End if` – Divin3 Dec 01 '16 at 15:37
@Divin3 - If your tests are structured property, then why are you testing `If isbackup = True` immediately after `isbackup = True`? Also, the `If isbackup = True` isn't consistently indented, which leads me to believe that it isn't intended to be inside the `If LCase(Cells(i, y).Value) = "backup" Then` block. – Comintern Dec 01 '16 at 15:43
@Comintern @D.O. You're right in the `isbackup` part. Removed it. – Divin3 Dec 01 '16 at 15:52
It is not currency, it is a special kind of value that needs to be different from the one from 2 columns left to it so I use the character `$` it could have been any other character, I just chose "$" by random. To be more specific I store PC names as hyperlinks in my list in a column that I can connect remotely if I click them, and 2 columns right are hyperlinks that point to their backup. The hyperlinks are generated after the PC names. – Divin3 Dec 01 '16 at 17:41
I think it is better to take another character. Because, when I have tested your code and take some examples with numbers and add "$" at the right, the cells with a number and "$" are modified to currency. So, checking what is the last character will give us the last number and not the "$" at the end. So if you have numbers change "$" to another character. – D. O. Dec 01 '16 at 18:46

Excel VBA bug/anomaly - ActiveWorkbook.Save changes Workbook_BeforeSave function

2 Answers2

Linked