Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

14119 #2277

Closed
wants to merge 5 commits into from

3 participants

@smfontes

These are the changes required to fix defect 14119 which I opened a few days ago. The problem was that all of the $INFO[slideshow.*] variables were blank when used in SlideShow.xml.
There were two problems.
The first problem was an incorrect condition in an if statement in GUIInfoManager.cpp. The condition was always false, so no metadata would be loaded from the picture file when going through this code path. I fixed the condition.
The second problem was that when metadata was explicitly set using PictureInfoTag.SetInfo(), it would cause all future calls to load the metadata from the picture file to be ignored. I created a new private variable to distinguish between setting metadata with SetInfo(), and setting metadata by loading it from the picture file. I used the variable to allow loading of metadata from the picture file even after some metadata had been set using SetInfo(). The metadata provided by SetInfo() is used if it does not appear in the file. If the data exists in the file, it overrides that provided by SetInfo().

@jmarshallnz jmarshallnz commented on the diff
xbmc/pictures/PictureInfoTag.cpp
((25 lines not shown))
if (exifDll.process_jpeg(path.c_str(), &m_exifInfo, &m_iptcInfo))
m_isLoaded = true;
+ /* Prefer to use the actual file metadata, but if it doesn't exist */
+ /* put the externally set metadata back in. */
+ if (m_isExternallyLoaded){
+ if ((GetInfo(TranslateString("exiftime")) == "") && (exiftime != ""))
+ SetInfo(TranslateString("exiftime"),exiftime);
+ if ((GetInfo(TranslateString("resolution")) == "") && (resolution != ""))
+ SetInfo(TranslateString("resolution"),resolution);
+ }
+
@jmarshallnz Owner

Please use xbmc formatting conventions, e.g. separate line for braces.

Where is exiftime and resolution set currently? Are others set from the same spot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz
Owner

Mind rebasing down to a single commit or two?

@smfontes

The exiftime and resolution values are currently set in the same file (PictureInfoTag.cpp), way at the bottom in the SetInfo() method. There is a switch statement that only handles setting exiftime and resolution. If SetInfo is called with any value other than those two, nothing happens. I think this is to handle the case where pictures were created using a scanner rather than a camera. An addon can get the info from the file attributes, then call SetInfo to make sure the picture has at least those two useful pieces of metadata.

I don't mind rebasing down to a single commit, but this is my first time using Git. Can you give me a simple set of git command line commands to do it? If you can't, I'll figure it out.

@smfontes smfontes closed this
@ulion
Collaborator

git rebase -i commit_id
will let you squash/fixup/reorder all commits after the commit_id.
git push yourgitusername branchname --force
will force push your local git to update the commits in the pr, so you can reuse the same pr and add additional commit or change existed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 22, 2013
  1. Fixes for bug 14119

    sfontes authored
  2. Fixes for bug 14119

    sfontes authored
  3. Formatting

    sfontes authored
  4. Formatting

    sfontes authored
  5. Formatting

    sfontes authored
This page is out of date. Refresh to see the latest.
View
2  xbmc/GUIInfoManager.cpp
@@ -5012,7 +5012,7 @@ void CGUIInfoManager::SetCurrentSlide(CFileItem &item)
{
if (m_currentSlide->GetPath() != item.GetPath())
{
- if (!item.HasPictureInfoTag() && !item.GetPictureInfoTag()->Loaded())
+ if (!item.GetPictureInfoTag()->Loaded()) // Get metadata from file if not yet loaded
item.GetPictureInfoTag()->Load(item.GetPath());
*m_currentSlide = item;
}
View
26 xbmc/pictures/PictureInfoTag.cpp
@@ -31,6 +31,7 @@ void CPictureInfoTag::Reset()
memset(&m_exifInfo, 0, sizeof(m_exifInfo));
memset(&m_iptcInfo, 0, sizeof(m_iptcInfo));
m_isLoaded = false;
+ m_isExternallyLoaded = false; // Distiguish between loaded from file and loaded by external call to SetInfo
}
const CPictureInfoTag& CPictureInfoTag::operator=(const CPictureInfoTag& right)
@@ -39,20 +40,39 @@ const CPictureInfoTag& CPictureInfoTag::operator=(const CPictureInfoTag& right)
memcpy(&m_exifInfo, &right.m_exifInfo, sizeof(m_exifInfo));
memcpy(&m_iptcInfo, &right.m_iptcInfo, sizeof(m_iptcInfo));
m_isLoaded = right.m_isLoaded;
+ m_isExternallyLoaded = right.m_isExternallyLoaded;
return *this;
}
bool CPictureInfoTag::Load(const CStdString &path)
{
+ CStdString exiftime = "";
+ CStdString resolution = "";
m_isLoaded = false;
DllLibExif exifDll;
if (path.IsEmpty() || !exifDll.Load())
return false;
+ /* Its possible that exiftime and resolution were already set externally */
+ /* Retrieve them so they can be used if not found in file */
+ if (m_isExternallyLoaded){
+ exiftime = GetInfo(TranslateString("exiftime"));
+ resolution = GetInfo(TranslateString("resolution"));
+ }
+
if (exifDll.process_jpeg(path.c_str(), &m_exifInfo, &m_iptcInfo))
m_isLoaded = true;
+ /* Prefer to use the actual file metadata, but if it doesn't exist */
+ /* put the externally set metadata back in. */
+ if (m_isExternallyLoaded){
+ if ((GetInfo(TranslateString("exiftime")) == "") && (exiftime != ""))
+ SetInfo(TranslateString("exiftime"),exiftime);
+ if ((GetInfo(TranslateString("resolution")) == "") && (resolution != ""))
+ SetInfo(TranslateString("resolution"),resolution);
+ }
+
@jmarshallnz Owner

Please use xbmc formatting conventions, e.g. separate line for braces.

Where is exiftime and resolution set currently? Are others set from the same spot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return m_isLoaded;
}
@@ -267,7 +287,7 @@ void CPictureInfoTag::GetStringFromArchive(CArchive &ar, char *string, size_t le
const CStdString CPictureInfoTag::GetInfo(int info) const
{
- if (!m_isLoaded)
+ if (!m_isLoaded && !m_isExternallyLoaded)
return "";
CStdString value;
@@ -590,12 +610,14 @@ void CPictureInfoTag::SetInfo(int info, const CStdString& value)
{
m_exifInfo.Width = atoi(dimension[0].c_str());
m_exifInfo.Height = atoi(dimension[1].c_str());
+ m_isExternallyLoaded = true;
}
break;
}
case SLIDE_EXIF_DATE_TIME:
{
strcpy(m_exifInfo.DateTime, value.c_str());
+ m_isExternallyLoaded = true;
break;
}
default:
@@ -605,6 +627,6 @@ void CPictureInfoTag::SetInfo(int info, const CStdString& value)
void CPictureInfoTag::SetLoaded(bool loaded)
{
- m_isLoaded = loaded;
+ m_isExternallyLoaded = loaded; // Don't set m_isLoaded variable. That indicates loaded from file
}
View
1  xbmc/pictures/PictureInfoTag.h
@@ -107,5 +107,6 @@ class CPictureInfoTag : public IArchivable, public ISerializable, public ISortab
ExifInfo_t m_exifInfo;
IPTCInfo_t m_iptcInfo;
bool m_isLoaded;
+ bool m_isExternallyLoaded; // Distiguish between loaded from file and loaded by external call to SetInfo
};
Something went wrong with that request. Please try again.