UT-206: Adding soft-delete of thumbnails

[andr9528]

Created as a draft PR, so developers (mainly Kasper) can come with feedback on changes prior to full on PR review.

PR reviewed for:

• UT-206
  • UT-207
  • UT-208
  • UT-209

[LindvedKrvang]

This is overengineered.
I see what you are trying to achieve, but it's enough to just extract the conditional:

function doesChannelExist(state: StateType, action: ActionType): boolean { // or whatever the conditional is trying to find out
  return state[0].output.length >= action.channelIndex
}

So now your switch cases would just look like this:

case: IO.SET_TIME:
  if (doesChannelExist(nextState, action)) {
    nextState[0].output[action.channelIndex].time = action.time
  }
  return nextState
}

[andr9528]

Noted, and Applied.

[LindvedKrvang]

isHidingStyle is using the naming convention for returning a boolean. This is returning an object, not a boolean.
Either rename the function or make it return a boolean.

Also, remember to specify return type

[andr9528]

Name updated to not include 'is', match new 'visibility' naming, and type added.

[LindvedKrvang]

HIDE_STATE is a weird phrasing especially when the english language offers words that describe this very thing. (HIDDEN or VISIBILITY - I vote for visibility; then it would be UPDATE_VISIBILITY which is more like normal english).

[andr9528]

Updated to use 'VISIBILITY' instead of 'HIDE'.

[LindvedKrvang]

If you go with VISIBILITY instead of HIDE then this would be SET_VISIBILITY which would make more sense since hide is a verb and you don't set a verb, you do a verb.

[andr9528]

Updated

[LindvedKrvang]

Continuing the visibility path, this would be:

export const setVisibility = (channelIndex: number, isVisible: boolean) => {
  return {
    type: SET_VISIBILITY,
    channelIndex,
    visibility: isVisible
  }
}

[andr9528]

Updated

[KvelaGorrrrnio]

A lot of the comments are stuff we already talked about today :)

[KvelaGorrrrnio]

Can be joined to a single line.

[andr9528]

Will try, but if the styling applied on commit is the one to do it, then i might not be able.

[KvelaGorrrrnio]

Is visibilityState a boolean representing whether we are in "edit visibility" mode or not?
If so, then should this be per output or defined as an application state?

I'll suggest to have an enum like

enum operationsMode {
  CONTROL = 'control',
  EDIT_VISIBILITY = 'edit-visibility',
}

to ensure a only one mode of operation being active, even if we introduce another "edit XXX" mode.

[andr9528]

Should have succesfully updated the code to make use of the suggested 'OperationMode' enum, instead of a boolean value.

[LindvedKrvang]

Is it really necessary to import the CSS file?

I guess it's because the CSS file has been moved to a separate folder than the tsx file

[andr9528]

Didn't work without the import. Also how it is setup in the other files, so it makes sense for it to be needed.

[KvelaGorrrrnio]

I don't think that React imports the CSS file automatically. It requires an import statement (either in css or js).

[LindvedKrvang]

What's the reasoning for having translations here? (And what's the reasoning for making a Map with one entry for translations?)

Ideally, components shouldn't know anything about translations (except for the translationId of the text it want to display).

Has it been requested that it should be translated?

[andr9528]

I'd not exactly call it translations. It is just how i chose to keep the different strings, for the description and button text, with their Operation Mode key, in a manner that would be easily expandable, should the need arise.

[KvelaGorrrrnio]

If the footer is where this should be living, then I would suggest creating an <OperationMode>Footer component, that encapsulates the logic for the corresponding operations mode.

[andr9528]

Footer has been split into multiple smaller components. Among those a component whos sole purpose is to contain the logic and constants specific for OperationMode.EDIT_VISIBILITY`. This eliminates the use of the map.

[LindvedKrvang]

Don't nest ternaries. They become way too complex to read way too fast.

Also, don't overcomplicate it more than necessary:

renderExitButton = () => {
  return (
    <div className='Footer-text'>
      You are currently editing visibility of files.
    </div>
    <button onClick={resetOperationMode} className='Footer-buton'>
      Exit Edit Visibility
    </button>
  )
}

return (
  <Footer className='Footer Footer-flex'>
    { if(operationMode !== OperationMode.CONTROL) {
        renderExitButton()
      }
    }
  </Footer>
)

[andr9528]

I have extracted the innards of the Footer to a seperate functional component, in this file. This eliminates the nested ternaries.
I am however keeping the Map usage, as it makes adding new operation modes easier, while not increasing the complexity by that THAT much, mostly due to the low size of the file.

On another note, if i were to return the capital letter Footer, i would be returning the same JSX Element, as the enclosing component, unless you also had a renaming of the component in mind?

[andr9528]

With my comment on one of the other footer chains (#78 (comment)), my last comment has become outdated.

[LindvedKrvang]

React components are spelled with the first letter uppercased to differentiate them from the regular HTML tags.
CSS classes are all lowercased.
So this should be:
<Footer className='footer'>

[andr9528]

Anders pointed this out too, to which i pointed out that many other CSS classes are spelled with a capital letter in this application, at the moment.

I would very much like to after this PR, to go through and update the code styling to be unified in how it is defined, along with a few other things that Anders and I have talked with Kasper about.
I am fairly certain i know how we, as a team, want the style to be, but i would be happy to have a chat about it, if you'd like.

[KvelaGorrrrnio]

@LindvedKrvang Just a small correction to your suggestion. The tag name should still be <footer className="footer">, since it is a native HTML5 component/tag that is used. Lowercasing the class name should be done, but as @andr9528 points out, the currently used style for component class names are capitalized.

[LindvedKrvang]

You are not toggling. You are changing OperationMode. Toggling is on/off.
A more accurate name for this method would be emitNewOperationMode or emitSelectedOperationMode or emitOperationModeChanged

[andr9528]

If i am in any other mode, it turns on 'Edit Visibility' mode, otherwise it turns off 'Edit Visibility' mode, by setting the mode to the default one, being 'Control'.
Maybe creating a OperationModeService class which has function for setting the mode to each posible one, could help with simplifying this.

[LindvedKrvang]

No matter what mode you are in you are just emitting an event here.
What the listeners of said event do (which may very well be toggling) has nothing to do with thehandleToggleEditVisibilityMode() function here, hence the name of this function should not indicate anything else than emitting an event.

[andr9528]

Renamed to emitSetOperationMode. Still not entirely sure if that name is descriptive enough.

[LindvedKrvang]

Don't use any type

[andr9528]

It is that, or using a type that would fill a couple of lines.
I am also not entirely sure if it would need to be a partial or not.

I would very much like to spend the time putting type of everything, while redoing the code structure.
This would be one of the areas that i'd spent some time improving.

[KvelaGorrrrnio]

It would be nice to at least have the minimum type declaration if not the State type or action type is avaiable.

function doesChannelExist(nextState: { output: unknown[] }[], action: { channelIndex: number }): boolean {
    return nextState[0].output.length > action.channelIndex
}

[andr9528]

Have added what you suggested Anders, with a slight improvement, from an unknown[] to an IOutput[].

[LindvedKrvang]

It is that, or using a type that would fill a couple of lines.

It's Typescript so just write the types needed for your actual function and nothing more (as Anders points out).
Ideally there would be an interface that all States uses and the same for Actions

[LindvedKrvang]

What was wrong with returning immediatly?

[andr9528]

By returning immediatly AND removing the new single return at the bottom, a default case is needed, which only returns the nextState.
If the single return at the bottom remains, the default case is gone. However in this case immediatly returning has the same effect as simly breaking, but at the cost of a longer statement.

If there was just ONE case where the state was not returned, then yes i would be returning in each case. But as it is, it is ALWAYS returned, so having one return statement at the end simplifies the code, as there at the end is only one path out of the function.

[KvelaGorrrrnio]

There are pros and cons for both approaches – personally I am in favor of the local returns. In tandem with my personal preference, I would like to point out that break indicates that something happens after the switch statement, which it is not the case here. So it weakens the locality of the execution flow, in contrast to a local return that clearly indicates that the execution flow for the function ends in that particular switch-case.

Using break with a single return also forces a stricter dependency on the variable nextState making the individual case less flexible in terms of what to return. It can both be a good or bad thing depending on how much control you want over the individual switch case.

[andr9528]

I think it would be nice to get input from @olzzon and @RasmusAlbrektsen on this one.
If either, also prefer the 'return-immediately' in this scenario, then i will change it back.

[andr9528]

After a chat with Kasper, i found that the original structure that he had developed, was a replica of the structure used in Redux documentation, thus it now makes more sense to be why it was used. I will be reverting this switch change, anywhere where Redux is part of it.

[LindvedKrvang]

If there was just ONE case where the state was not returned, then yes i would be returning in each case. But as it is, it is ALWAYS returned, so having one return statement at the end simplifies the code, as there at the end is only one path out of the function.

I disagree. As Anders points out, by not returning you indicate that more is going to happen so you have to read the rest of the method. It doesn't get much simpler than a return statement saying "now we are done"

[LindvedKrvang]

This function is called toggle, but are you both CREATING and DELETING which is something completely different than toggling

[andr9528]

I am toggleing, wheather the file in question is in the record of hidden files, by either deleting a record entry (turning off) or creating a record entry (turning on).

The method was build together with Anders, where we did extract the responsibility of building the new record out into another function.
Not sure how i'd better split the responsibilities in a more meaningfull way.

[KvelaGorrrrnio]

You could extract the logic for hiding and showing the files into their own functions like so:

type IHiddenFiles = Record<string, IHiddenFileInfo>

function toggleHiddenFile(fileName: string, channelIndex: number, hiddenFiles: IHiddenFiles): IHiddenFiles {
    return isFileHidden(fileName, hiddenFiles)
        ? showFile(fileName, hiddenFiles)
        : hideFile(fileName, channelIndex, hiddenFiles)
}

function isFileHidden(fileName: string, hiddenFiles: IHiddenFiles): boolean {
    return fileName in hiddenFiles
}

function showFile(fileName: string, hiddenFiles: IHiddenFiles): IHiddenFiles {
    const { [fileName], ...newHiddenFiles } = hiddenFiles
   return newHiddenFiles
}

function hideFile(fileName: string, channelIndex: number, hiddenFiles: IHiddenFiles): IHiddenFiles {
    const hiddenFile = getHiddenFileMetadataFromFileName(filename, channelIndex)
    return { ...hiddenFiles, [fileName]: hiddenFile }
}

[andr9528]

Reworked the function to use most of Anders' suggestion.

[LindvedKrvang]

export const hasThumbListChanged = (...): boolean => {
   ...
 
  return newList.some((entry, index) => entry.name !== previousList[i].size || entry.name !== previousList[i].size)
}

[andr9528]

Good to learn that the Typescript Arrays' .some(...) is the quivialant of LINQ's .Any(...).

Have applied the suggest change, with a bit more styling, for readability.

[LindvedKrvang]

FYI, most of the operations of LINQ is found on Array

[LindvedKrvang]

What does this JSON do?

[andr9528]

It is a file generated by Cliptool now, that contains a Record of which files are hidden, with their size and changed values.

[LindvedKrvang]

Why not just use localStorage?

[KvelaGorrrrnio]

You have made some good improvements and I found a little more that needs som love ❤️

[KvelaGorrrrnio]

@LindvedKrvang Just a small correction to your suggestion. The tag name should still be <footer className="footer">, since it is a native HTML5 component/tag that is used. Lowercasing the class name should be done, but as @andr9528 points out, the currently used style for component class names are capitalized.

[KvelaGorrrrnio]

If the footer is where this should be living, then I would suggest creating an <OperationMode>Footer component, that encapsulates the logic for the corresponding operations mode.

[KvelaGorrrrnio]

Return type for this would be nice.

[andr9528]

Added return type and updated to be a function definition instead of constant.

[KvelaGorrrrnio]

You could extract the logic for hiding and showing the files into their own functions like so:

type IHiddenFiles = Record<string, IHiddenFileInfo>

function toggleHiddenFile(fileName: string, channelIndex: number, hiddenFiles: IHiddenFiles): IHiddenFiles {
    return isFileHidden(fileName, hiddenFiles)
        ? showFile(fileName, hiddenFiles)
        : hideFile(fileName, channelIndex, hiddenFiles)
}

function isFileHidden(fileName: string, hiddenFiles: IHiddenFiles): boolean {
    return fileName in hiddenFiles
}

function showFile(fileName: string, hiddenFiles: IHiddenFiles): IHiddenFiles {
    const { [fileName], ...newHiddenFiles } = hiddenFiles
   return newHiddenFiles
}

function hideFile(fileName: string, channelIndex: number, hiddenFiles: IHiddenFiles): IHiddenFiles {
    const hiddenFile = getHiddenFileMetadataFromFileName(filename, channelIndex)
    return { ...hiddenFiles, [fileName]: hiddenFile }
}

[KvelaGorrrrnio]

I have approved on the assumption that the following comments are addressed.
Nice work! 🚀

[KvelaGorrrrnio]

Since these are constants, they should be renamed to uppercased snake-case, e.g. BUTTON_TEXT.

[andr9528]

Have been renamed to use SNAKE_CASE.

[KvelaGorrrrnio]

Just a note for the refactor: Use single quotes for strings :)

[andr9528]

Think those were copied directly from the assignment to className, where it does make use of double quotes. Have updated it in 3 locations to use single quotes.

[KvelaGorrrrnio]

It would be nice to at least have the minimum type declaration if not the State type or action type is avaiable.

function doesChannelExist(nextState: { output: unknown[] }[], action: { channelIndex: number }): boolean {
    return nextState[0].output.length > action.channelIndex
}

[KvelaGorrrrnio]

I will like to challenge the buildX naming convention. How is it different from getX?
At least we should have a consistent way of naming.

[LindvedKrvang]

Nothing is being built here. At most your are wrapping the parameter in an object.
I would suggest to use getMetaData(file: IMediaFile) which clearly indicates that you are extracting the meta data from the file your are passing.

Also, no reason to include 'Hidden' in the name. The function doesn't care about if the file is hidden or not, it just need to be a file.

[andr9528]

Have renamed the commented on method to getMetaData(...).

[LindvedKrvang]

Your whole Footer components are way overdone. You are using three components to write:

export function Footer(): JSX.Element {
  resetOperationMode() {...}

  renderExitEditButton = () => {
    return (
      <div className='Footer-text'>
        You are currently editing visibility of files.
      </div>
      <button onClick={resetOperationMode} className='Footer-buton'>
        Exit Edit Visibility
      </button>
    )
  }
  
  return (
    <footer className='Footer Footer-flex'>
      { if(operationMode !== OperationMode.CONTROL) {
          renderExitEditButton()
        }
      }
    </footer>
  )
}

This is all you need. EditVisibilityFooter and FooterContent are way overkill. They do basically nothing.

[LindvedKrvang]

It would be nice if you would place the Component as the first function of the file and the place the helper functions below it in the order they are called.
There is a tendency in the JavaScript/TypeScript community to not do it like this so this is mostly a personal preference.
The idea behind this is to get a natural flow of reading in the file. When I go to a file called CarOverview.jsx I don't want to see the intriciaties of how they calculated the milage as the very first thing. I want to see the actual overview of the car and then start narrowing down to the specific thing I am after.
It's easier to express verbally so if you need a better explanation don't hesitate to ask

[LindvedKrvang]

This div should technically be a label.

Also, the identation seems to be misaligned (that may or may not just be GitHub showing it wrong, but I think it's not)

[andr9528]

The file in question (and lines) have had mayor changes performed on the branch for UT-211.

Needs to question Kasper/Jacob/Kevin if we want to release the new and improved tabs too. If not, then changes to the Footer should very likely be cherry-picked onto this branch.

[LindvedKrvang]

This class is called Thumbnail.tsx which means the only thing that should be exported from this class is the Thumbnail component, which this is not (same goes for findThumbPix)
If you need utility functions that are used several places you should create a dedicated class for that. If the functions are just used in here, they should not be exported.

[andr9528]

Not sure why Kasper added export to isThumbWithTally. If he had only one component per file, he'd need it to be exported as it is used by multiple components in this file. As it is, it isn't needed, and Cliptool builds just fine, if it is not exported.

In the case of findThumbPix (now renamed to findThumbnail), it is also used by the header. Havn't looked deeply into where, what or why. When splitting the file into multiple files for UT-210, this will be fixed.

[LindvedKrvang]

What does 'Pix' mean? Is it Picture, Pixel or something third?
The function is returning a string which appears to be a thumbnail, so wouldn't the name for this method be 'findThumbnail'?

[andr9528]

Renamed to findThumbnail.

[LindvedKrvang]

This relationship seems to be flipped. CommonFile sounds like is stuff that is common for all Files, including hidden Files. Hence it should be HiddenFileInfo that extends CommonFile and not the other way around.

What you have done is make IMedaieFile and IThumbFile be hidden File which I hope was intentional. But if they need to be hidden aswell that just argues more in favour for my other comment about just adding the hidden attribute to the File interface

[LindvedKrvang]

Also, just call it File instead of CommonFile

[andr9528]

Have updated the name to IFile. Cleaning up the use of 'I' on interfaces will be handled during UT-210.

Attributes on HiddenFileInfo are data relevant to a hidden file, and nothings more. The Attributes of HiddenFileInfo also need to exist on the different files (IMediaFile / IThumbnailFile), as it is these attributes that gets extracted and persisted to figure out which version of a file is saved.

Attributes on IFile are data relevant to IMediaFile / IThumbnailFile, while in Cliptool. These attributes on IFile are irrelevant to determin the version of the file, when checking if the file has changed outside Cliptool.

[LindvedKrvang]

ThumbnailFile, not ThumbFile

[andr9528]

Has been renamed. Keeping the I, for now, as it will be handled as part of UT-210.

[LindvedKrvang]

You have an interface dedicated to only add this attribute. Because of that, this attribute should not be optional.
But since a big part of ClipTool is thumbnail management then there is no reasong for thumbnail not to be on the CommonFile interface

[andr9528]

Not sure why Kasper originally made that attribute optional. Removing the ability for it to be optional doesn't appear to break anything.

As for moving it to the ICommonFile interface, i have some questions that would need answer before doing that. In case it is moved, then...

• I take that thumbnailList on IOutput should have its type changed to ICommonFile?

When media files are retrived from CCG, they do not include the thumbnails, those are retrieved in a seperate call to CCG. The code would have to be updated to merge the thumbnail data from the one call, into the media files from the other call.

This is well out of scope for this task, i'd say, even if considered as boy-scouting.

[olzzon]

@andr9528 I can't recall why i made it optional.
Could be related to whether the CasparCG scanner is running or not, or som other case. But it could also be an old reminiscence of something.
As you point out the reason for having it separate is that it's received separate from CCG. The other plan for it, was as far as i remember to have all media thumbnails available on the server, but only the one needed in the client. But it's been a long time since i looked at it, so if you wan't to know for sure, I need to spend a couple of hours digging in to it.

[andr9528]

@olzzon I'm not going to be moving that attribute for this PR.
It would be a nice-to-know for in the future, but no rush.

Also that was a quick response from you. I didn't even ping you.

[LindvedKrvang]

Does this really need to be a Record?

[andr9528]

I tried with a Map at first, but couldn't get it to work, so Anders came and helped me, and together we found out that Redux doesn't support maps. Redux does however support Records.

[LindvedKrvang]

It is that, or using a type that would fill a couple of lines.

It's Typescript so just write the types needed for your actual function and nothing more (as Anders points out).
Ideally there would be an interface that all States uses and the same for Actions

[LindvedKrvang]

In general you should try to avoid the use of loops (they do have their place, but it's becoming more rare). You can use Array.some() for the same behaviour

function isThumbnailWithAllayOnAnyOutput(...): boolean {
  return reduxState.media[0].ouput.some(output => getCleanTallyFile(output) === thumbnailName)
}

[andr9528]

Have applied the suggested change.

In the case of the loop, it did exit early, just as the .some(...) does, the loop just had a larger footprint. Not saying the loop is better - just pointing out that it did exit early.

[LindvedKrvang]

I'm taking a guess here and assume you want it to say as a file?

[andr9528]

The alert is triggered when a user clicks to try and hide a file which currently is in use on another output. Thus it is 'Unable to hide, as the file is in use somewhere.' Added a 'the' to further specify that the file in question is the one clicked.

[LindvedKrvang]

That still worries me and suggest to me that there is somewhere else the CSS isn't right, since this is needed...

[LindvedKrvang]

Since you are already here refactoring, you should also remove the "I" prefix

[andr9528]

'I' prefix has been removed.