Commit f11601a3 authored by Akarsh Simha's avatar Akarsh Simha

Fix crash in observing list whenever catalog configuration was changed.

a.k.a. the "Wishlist-Killer" bug.

A very frustrating crash would ensue whenever the catalog
configuration options were changed (eg: adding a new custom catalog)
and an object was added to the observing wishlist. Before a recent
commit, the wishlist would be truncated causing much frustration to
the user.

The crash occurred because the SkyObjects are de-allocated and
re-allocated by SkyMapComposite::reloadDeepSky() every time the
catalog configuration objects have changed. Since the ObservingList
maintained a QList<SkyObject *> pointers to these SkyObjects, we were
often querying dangling pointers, leading to crashes. So addition of
the object to the wishlist, although did trigger the bug, was not the
root cause.

This change fixes the bug (hopefully without inducing any
others!). The changes are:

1. Create a clone of the SkyObject in ObservingList

   This makes add / remove more complicated as they look-up the given
   object by name, but solves the crash because we have a local copy
   of the SkyObject even if the CatalogComponent's copy is deleted.

2. Use QSharedPointer<SkyObject> instead of SkyObject *

   This makes sure that we don't re-create the same object for session
   plan / wish list. Besides, it is much more cleaner, and takes care
   of deleting itself etc.

Extensive testing is appreciated.

And once again, happy 20th Birthday KDE!

BUG: 345348
CCMAIL: kstars-devel@kde.org
parent 42d4149a
......@@ -633,7 +633,7 @@ void KStars::renderEyepieceView( const QString &objectName, const QString &destP
}
QString KStars::getObservingWishListObjectNames() {
QString output;
foreach( const SkyObject *object, KStarsData::Instance()->observingList()->obsList() ) {
foreach( QSharedPointer<SkyObject> object, KStarsData::Instance()->observingList()->obsList() ) {
output.append( object->name() + '\n' );
}
return output;
......@@ -641,7 +641,7 @@ QString KStars::getObservingWishListObjectNames() {
QString KStars::getObservingSessionPlanObjectNames() {
QString output;
foreach( const SkyObject *object, KStarsData::Instance()->observingList()->sessionList() ) {
foreach( QSharedPointer<SkyObject> object, KStarsData::Instance()->observingList()->sessionList() ) {
output.append( object->name() + '\n' );
}
return output;
......
......@@ -218,8 +218,8 @@ void Execute::slotLocation() {
void Execute::loadTargets() {
ui.Target->clear();
sortTargetList();
foreach( SkyObject *o, KStarsData::Instance()->observingList()->sessionList() ) {
ui.Target->addItem( getObjectName(o, false) );
foreach( QSharedPointer<SkyObject> o, KStarsData::Instance()->observingList()->sessionList() ) {
ui.Target->addItem( getObjectName(o.data(), false) );
}
}
......@@ -245,23 +245,22 @@ void Execute::loadObservers() {
}
void Execute::sortTargetList() {
qSort( KStarsData::Instance()->observingList()->sessionList().begin(), KStarsData::Instance()->observingList()->sessionList().end(), Execute::timeLessThan );
}
bool Execute::timeLessThan ( SkyObject *o1, SkyObject *o2 )
{
QTime t1 = KStarsData::Instance()->observingList()->scheduledTime( o1 );
QTime t2 = KStarsData::Instance()->observingList()->scheduledTime( o2 );
if( t1 < QTime(12,0,0) )
t1.setHMS( t1.hour()+12, t1.minute(), t1.second() );
else
t1.setHMS( t1.hour()-12, t1.minute(), t1.second() );
if( t2 < QTime(12,0,0) )
t2.setHMS( t2.hour()+12, t2.minute(), t2.second() );
else
t2.setHMS( t2.hour()-12, t2.minute(), t2.second() );
return ( t1 < t2 ) ;
auto timeLessThan = []( QSharedPointer<SkyObject> o1, QSharedPointer<SkyObject> o2 ) {
QTime t1 = KStarsData::Instance()->observingList()->scheduledTime( o1.data() );
QTime t2 = KStarsData::Instance()->observingList()->scheduledTime( o2.data() );
if( t1 < QTime(12,0,0) )
t1.setHMS( t1.hour()+12, t1.minute(), t1.second() );
else
t1.setHMS( t1.hour()-12, t1.minute(), t1.second() );
if( t2 < QTime(12,0,0) )
t2.setHMS( t2.hour()+12, t2.minute(), t2.second() );
else
t2.setHMS( t2.hour()-12, t2.minute(), t2.second() );
return ( t1 < t2 ) ;
};
qSort( KStarsData::Instance()->observingList()->sessionList().begin(), KStarsData::Instance()->observingList()->sessionList().end(), timeLessThan );
}
void Execute::addTargetNotes() {
......
......@@ -89,13 +89,6 @@ Q_OBJECT
*/
void sortTargetList();
/** @short Custom LessThan function for the sort by time
* This compares the times of the two skyobjects
* using an edited QTime as next day 01:00 should
* come later than previous night 23:00
*/
static bool timeLessThan( SkyObject *, SkyObject * );
/** @short set the currentTarget when the user selection
* is changed in the target combo box
*/
......
......@@ -18,6 +18,7 @@
#include "oal/log.h"
#include "kstars.h"
#include "ksutils.h"
#include "kstarsdata.h"
#include "skyobjects/skyobject.h"
#include "skyobjects/starobject.h"
......@@ -30,7 +31,7 @@
void OAL::Log::writeBegin()
{
output = "";
m_targetList = KStarsData::Instance()->observingList()->sessionList();
m_targetList = KSUtils::makeVanillaPointerList( KStarsData::Instance()->observingList()->sessionList() );
writer = new QXmlStreamWriter(&output);
writer->setAutoFormatting( true );
writer->writeStartDocument();
......
......@@ -24,6 +24,7 @@
#include "kstarsdata.h"
#ifndef KSTARS_LITE
#include "skymap.h"
#include "ksutils.h"
#endif
#include "skyobjects/starobject.h"
#include "skyobjects/deepskyobject.h"
......@@ -287,10 +288,14 @@ void SkyMapComposite::draw( SkyPainter *skyp )
// map->infoBoxes()->reserveBoxes( psky );
if( KStars::Instance() ) {
const QList<SkyObject*> obsList = KStarsData::Instance()->observingList()->sessionList();
auto& obsList = KStarsData::Instance()->observingList()->sessionList();
if( Options::obsListText() )
foreach( SkyObject* obj, obsList ) {
SkyLabeler::AddLabel( obj, SkyLabeler::RUDE_LABEL );
foreach( QSharedPointer<SkyObject> obj_clone, obsList ) {
// Find the "original" obj
SkyObject *o = findByName( obj_clone->name() ); // FIXME: This is sloww.... and can also fail!!!
if ( !o )
continue;
SkyLabeler::AddLabel( o, SkyLabeler::RUDE_LABEL );
}
}
......@@ -340,7 +345,7 @@ void SkyMapComposite::draw( SkyPainter *skyp )
m_ObservingList->pen = QPen( QColor(data->colorScheme()->colorNamed( "ObsListColor" )), 1. );
if( KStars::Instance() && !m_ObservingList->list )
m_ObservingList->list = &KStarsData::Instance()->observingList()->sessionList();
m_ObservingList->list = new SkyObjectList( KSUtils::makeVanillaPointerList( KStarsData::Instance()->observingList()->sessionList() ) ); // Make sure we never delete the pointers in m_ObservingList->list!
if( m_ObservingList )
m_ObservingList->draw( skyp );
......
This diff is collapsed.
......@@ -94,22 +94,17 @@ public:
*/
~ObservingList();
/** @return true if the object is in the observing list
*@p o pointer to the object to test.
*/
inline bool contains( const SkyObject *o ) { return m_WishList.contains( const_cast<SkyObject*>(o) ); }
/** @return true if the window is in its default "large" state.
*/
bool isLarge() const { return bIsLarge; }
/** @return reference to the current observing list
*/
QList<SkyObject*>& obsList() { return m_WishList; }
QList<QSharedPointer<SkyObject>>& obsList() { return m_WishList; }
/** @return reference to the current observing list
*/
QList<SkyObject*>& sessionList() { return m_SessionList; }
QList<QSharedPointer<SkyObject>>& sessionList() { return m_SessionList; }
/** @return pointer to the currently-selected object in the observing list
*@note if more than one object is selected, this function returns 0.
......@@ -189,13 +184,20 @@ public:
*/
QString getObjectName(const SkyObject *o, bool translated=true);
/**
* @return true if the selected list contains the given object
*/
inline bool contains( const SkyObject *o, bool session = false ) { return bool( findObject( o, session ) ); }
QSharedPointer<SkyObject> findObject( const SkyObject *o, bool session = false );
public slots:
/** @short add a new object to list
*@p o pointer to the object to add to the list
*@p session flag toggle adding the object to the session list
*@p update flag to toggle the call of slotSaveList
*/
void slotAddObject( SkyObject *o=NULL, bool session=false, bool update=false );
void slotAddObject( const SkyObject *o=NULL, bool session=false, bool update=false );
/** @short Remove skyobjects which are highlighted in the
*observing list tool from the observing list.
......@@ -209,7 +211,7 @@ public slots:
*@p update flag to toggle the call of slotSaveList
*Use SkyMap::clickedObject() if o is NULL (default)
*/
void slotRemoveObject( SkyObject *o=NULL, bool session=false, bool update=false );
void slotRemoveObject( const SkyObject *o=NULL, bool session=false, bool update=false );
/** @short center the selected object in the display
*/
......@@ -368,7 +370,7 @@ private:
* @short Return the active list
* @return The session list or the wish list depending on which tab is currently being viewed.
*/
inline QList<SkyObject *>& getActiveList() { return ( ( sessionView ) ? m_SessionList : m_WishList ); }
inline QList<QSharedPointer<SkyObject>>& getActiveList() { return ( ( sessionView ) ? m_SessionList : m_WishList ); }
/**
* @short Return the active itemmodel
......@@ -396,7 +398,7 @@ private:
KSAlmanac *ksal;
ObservingListUI *ui;
QList<SkyObject*> m_WishList, m_SessionList;
QList<QSharedPointer<SkyObject>> m_WishList, m_SessionList;
SkyObject *LogObject, *m_CurrentObject;
bool isModified, bIsLarge, sessionView, dss, singleSelection, showScope, noSelection;
QString m_listFileName, m_currentImageFileName, ThumbImage;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment